Ayende @ Rahien

It's a girl

What is wrong with this API?

Originally posted at 11/11/2010

This is part of a base class, and we need to provide a way to veto messages for extensions.

image

Can you figure out what is wrong with this API?

Comments

Torkel
11/12/2010 10:18 AM by
Torkel

If the function only decides if a message should be vetoed or not it should reflect that. That is name the function ShouldVetoMessage

configurator
11/12/2010 10:20 AM by
configurator

What does true (or false) mean in the overriding function? A boolean seems out of place here. An enum would do better.

configurator
11/12/2010 10:22 AM by
configurator

(Or renaming the function in a more positive tone to avoid double negatives - ValidateMessage maybe or some such)

stoo
11/12/2010 10:26 AM by
stoo

You're using a class rather than an interface for the parameter type.. ?

(unless you've chosen IncomingMessage to be an abstract base class tfor all IncomingMessage types)

James Gregory
11/12/2010 10:33 AM by
James Gregory

The doc comment is confusing?

stoo
11/12/2010 10:35 AM by
stoo

... reading it again I think Torkel has it ...

the Function 'VetoMessage' is a verb and a noun ... so it sounds like it has side effects... it doesn't ..so it def' should be 'ShouldVetoMessage'

'ValidateMessage' would imply that it's performing validation.. so prob' wouldn;t go for that as there may be any number of reasons you might want to veto a valid message

Steve Py
11/12/2010 10:36 AM by
Steve Py

Hmm. +1 for confusing documentation.

Not sure but if this is dealing with an incoming message it might be tempting to attempt to call it from a base constructor, which would be bad.

Krzysztof Koźmic
11/12/2010 10:37 AM by
Krzysztof Koźmic

Other than as James noted the XML comment is confusing the API itself is perfectly fine

Andreyg
11/12/2010 10:39 AM by
Andreyg

The method name is confusing. Should be something like ShouldVetoMessage(...)

addy santo
11/12/2010 10:43 AM by
addy santo

all of the above, + if vetoing is important functionality then I would make this abstract. If the inheriting classes are written by the consumers (ie not Oren) then they should be forced to provide their own implementation, otherwise everyone will default to the "return false" :)

Rik Hemsley
11/12/2010 10:48 AM by
Rik Hemsley

It's implied by the fact it's not abstract that this implementation has real meaning. Does it really make sense for this particular class to make a decision on whether or not to veto? If not, it should be abstract.

Also, as others have noted, the XML comment is confusing. Its grammar is also broken. It should also be removed and the method renamed, as in this case it would be simple to name the method such that it was self-documenting.

So, as others have noted, it should be renamed, but 'ShouldVetoMessage' doesn't convey that it will not be 'enqueued'. So, based on the information provided, it's most likely it should be named 'ShouldBeEnqueued'.

Three WTFs, two lines of code. Impressive.

Steve Py
11/12/2010 11:03 AM by
Steve Py

Alternatively, I might opt instead for something along the lines of a "Vetoed" flag inside of IncomingMessage, similar to "Cancelled" or "Handled" in various event models. This way anything that touches something designated as an incoming message has the chance to veto it in whatever relevant call they can override.

George C.
11/12/2010 11:09 AM by
George C.

all of the above + use Nonvirtual interface idiom

Ayende Rahien
11/12/2010 11:20 AM by
Ayende Rahien

Rik,

This is the default impl, sub classes can choose to override this method or not.

The XML comment grammer is pretty much the standard for me, I am afraid, that isn't what I meant.

And the problem isn't in the method name either

Evgeny Shapiro
11/12/2010 11:30 AM by
Evgeny Shapiro

Why should one subclass? Interceptor might be a more appropriate solution.

Sean Kearon
11/12/2010 11:33 AM by
Sean Kearon

Usually you will accept the message, so the default should return false and let specialisms do the vetoing.

alwin
11/12/2010 11:36 AM by
alwin

Well for me it's not clear if true means that this message is passed or denied.

Maybe this would be better?

public virtual bool AllowMessage(IncomingMessage message){

return true;

}

Stefan Wenig
11/12/2010 11:52 AM by
Stefan Wenig

mind to give some context? the problem might be that you need to derive from some (what?) base class only to get to veto - and then maybe someone else derives too and now you got two competing implementations? might be better to enlist everyone whoi might need to veto, maybe using cancelable events.

Oleg
11/12/2010 11:53 AM by
Oleg

We cannot tell by looking at signature what exactly it's return value refers to?

Andrew
11/12/2010 11:57 AM by
Andrew

There's no way to explicitly allow a message (and prevent a veto)?

You may want a VetoResult { Permit, Deny, Ignored } result instead so that an extension can explicitly permit or deny the message from being propagated; or decide to have no action on the message (Ignored).

wouzer
11/12/2010 12:01 PM by
wouzer

Maybe you are missing the context to base the decision on?

public virtual bool VetoMessage(QueueContext context, IncomingMessage message)

{

return true;

}

Koen
11/12/2010 12:02 PM by
Koen

Veto-ing is about overruling in a hierarchical system. A concrete class can inherit the base class and execute logic to decide if a message is blocked as some sort of superior.

However, you can inherit from that class again, override the method again and at the same time overrule your "superior". I guess that's a flaw...

Dan Plaskon
11/12/2010 12:02 PM by
Dan Plaskon

I think Andrew's on the right track..I think more context would be helpful here...eg how is VetoMessage called/used for these plugins by the infrastructure?

Dan Plaskon
11/12/2010 12:05 PM by
Dan Plaskon

@Koen: Couldn't that be prevented by sealing the class though?

Nick
11/12/2010 12:29 PM by
Nick

I think the comment does not make sense.

Adam B
11/12/2010 12:36 PM by
Adam B

Violates Open/Closed principal? Looks open for modification.

Andrey Shchekin
11/12/2010 12:41 PM by
Andrey Shchekin

It does not allow to specify a reason?

Mike Minutillo
11/12/2010 12:47 PM by
Mike Minutillo

Callers will (potentially) be able to tell who vetoed a message but not why (unless you have some very fine grained message checkers). It would be better to have something like:

public virtual void VetoMessage(IncomingMessage message, VetoContext context) {

if( someCondition ) {

context.VetoMessage(someReason);

}

}

Trey
11/12/2010 01:29 PM by
Trey

The fact that the method is public seems to imply that the decision to veto can be bypassed (i.e. - It seems to be the responsibility of the caller to choose whether or not to evaluate the possibility of vetoing the message).

The exact context and responsibility of the example itself is unclear to me (is the method intended to actually perform the logic for vetoing the message, or simply decide whether or not the message should be vetoed.....what is the overall responsibility of the base class, etc.), so the decision to make this public could very well be by design. Just my humble two cents. :)

Brian Frantz
11/12/2010 01:43 PM by
Brian Frantz

Should be a protected method - meant for internal logic, not for the end-user of the class.

WishCow
11/12/2010 01:53 PM by
WishCow

The method's name is misleading, because VetoMessage(message) leads you to believe that you pass a message that you want to veto, but the doc comment states that the method will only decide if the message is vetoed, or not, and does not mark a message as vetoed.

The method should be renamed to isVetoed(message).

Jason Meckley
11/12/2010 02:13 PM by
Jason Meckley

My first thought was the context is missing. And I still think that is the number 1 problem with the method. After reading the comments I agree that the name of the message should be changed to ShouldVetoMessage.

VetoMessage sounds like this will veto, not whether is should veto.

jonnii
11/12/2010 03:05 PM by
jonnii

I know this has already been said but I believe that the reason is because it doesn't give the end user enough discovery as to why the message was vetoed.

I'd rather have something like (forgive the crappy names):

public virtual MessageVeto VetoMessage(IncomingMessage message){

return MessageVeto.MessageOk;

}

where MessageVeto is something like:

class VetoMessage{

static VetoMessage Ok(){...}

 string Reason { get; set; }

 bool IsVetoed { get { !string.IsNullOrEmpty(Reason); } }

}

This means if you're using the API for the first time and you're wondering why your messages are being vetoed you can look at the reason and make the change.

The other option is that you can have an enum as the return type which gives you more indication as to why the message was vetoed if there are classifications, which means you don't want to add another class, that's still more discoverable than a boolean and gives you options to add more reasons later without, hopefully, breaking the API.

If you absolutely must return a boolean for some reason you could add an event that gets raised when a message is vetoed with a reason.

jonnii
11/12/2010 03:07 PM by
jonnii

One more thing, this is very similar to what Mike suggested, but I prefer returning the result instead of putting it in a context because that way you can write:

if(foo.VetoMessage(m).IsVetoed){...}

instead of

var context = ..,;

foo.VetoMessage(m, context);

if(context.IsVetoed()){...}

And it means you can make the response immutable, another win for SCIENCE!

Richard Dingwall
11/12/2010 03:26 PM by
Richard Dingwall

If there are multiple extensions each with this method, which one do you listen to?

Tom
11/12/2010 04:11 PM by
Tom

Should return an int or enum for greater flexability

lowds
11/12/2010 05:22 PM by
lowds

Whether an incoming message has been vetoed or not represents a state of the message and this state would ideally be a property on the message itself.

Having a method on the IncomingMessage class that takes a list of extensions would solve this, something like:

message.AssertVeto(extensions);

(or what ever a better name for AssertVeto would be)

This would let the message track it's own state and then implement any additional logic required rather than have this in a seperate controller-like class. Any other class can then query the state of the message rather than pass around a boolean.

Equally if you wanted to have a message that cannot be vetoed you can create a subclass of IncomingMessage and override the AssertVeto implementation.

Jackknife
11/12/2010 06:23 PM by
Jackknife

Disencourages ask - don't tell?

Sony Mathew
11/12/2010 06:38 PM by
Sony Mathew

There is nothing technically wrong with this API. However, it would be better if this API were exposed via a modified Observer pattern.

E.g.

addVetoAuthority(VetoAuthority v)

where:

interface VetoAuthority {

boolean isVetoed(IncomingMessage m);

}

Sony

Michael D.
11/12/2010 07:21 PM by
Michael D.

Inability to chain filters that can prevent from the message from being sent?

Kevin L
11/12/2010 11:08 PM by
Kevin L

On the surface, this method looks fine, but a descendant class being allowed to override the method creates a grammatical ambiguity:

public bool override VetoMessage(,,,) {...}

By definition a veto cannot be over-ridden.

David Cuccia
11/13/2010 12:29 AM by
David Cuccia

The method doesn't make sense - if the caller has already decided to veto the message, why would it call a method just to get a boolean back to say that it was vetoed (or not).

Instead, the return type should be void and the default implementation should either do nothing or actually veto the message.

David Cuccia
11/13/2010 09:07 PM by
David Cuccia

(Or the method name should be TryVeto)

Comments have been closed on this topic.