Ayende @ Rahien

My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:


+972 52-548-6969

, @ Q c

Posts: 6,026 | Comments: 44,842

filter by tags archive



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)

James Gregory

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

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

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

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

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

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.

all of the above + use Nonvirtual interface idiom

Ayende Rahien


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

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

Sean Kearon

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;


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.


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

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

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


I think the comment does not make sense.

Adam B

Violates Open/Closed principal? Looks open for modification.

Andrey Shchekin

It does not allow to specify a reason?

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





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

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

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.


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:


instead of

var context = ..,;

foo.VetoMessage(m, context);


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

Richard Dingwall

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:


(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?

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.


addVetoAuthority(VetoAuthority v)


interface VetoAuthority {

boolean isVetoed(IncomingMessage m);



Michael D.

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

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

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

(Or the method name should be TryVeto)

Comment preview

Comments have been closed on this topic.


No future posts left, oh my!


  1. Technical observations from my wife (3):
    13 Nov 2015 - Production issues
  2. Production postmortem (13):
    13 Nov 2015 - The case of the “it is slow on that machine (only)”
  3. Speaking (5):
    09 Nov 2015 - Community talk in Kiev, Ukraine–What does it take to be a good developer
  4. Find the bug (5):
    11 Sep 2015 - The concurrent memory buster
  5. Buffer allocation strategies (3):
    09 Sep 2015 - Bad usage patterns
View all series


Main feed Feed Stats
Comments feed   Comments Feed Stats