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

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

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

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)

The doc comment is confusing?

... 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

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.

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

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

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" :)

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.

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.

all of the above + use Nonvirtual interface idiom

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

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

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

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;

}

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.

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

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).

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

public virtual bool VetoMessage(QueueContext context, IncomingMessage message)

{

return true;

}

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...

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?

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

I think the comment does not make sense.

Violates Open/Closed principal? Looks open for modification.

It does not allow to specify a reason?

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);

}

}

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. :)

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

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).

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.

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.

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!

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

Should return an int or enum for greater flexability

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.

Disencourages ask - don't tell?

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

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

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.

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.

(Or the method name should be TryVeto)