http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660(Or the method name should be TryVeto) http://ayende.com/4689/what-is-wrong-with-this-api#comment43http://ayende.com/4689/what-is-wrong-with-this-api#comment43Sat, 13 Nov 2010 21:07:23 GMTThe 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. http://ayende.com/4689/what-is-wrong-with-this-api#comment42http://ayende.com/4689/what-is-wrong-with-this-api#comment42Sat, 13 Nov 2010 00:29:27 GMTOn 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. http://ayende.com/4689/what-is-wrong-with-this-api#comment41http://ayende.com/4689/what-is-wrong-with-this-api#comment41Fri, 12 Nov 2010 23:08:44 GMTInability to chain filters that can prevent from the message from being sent? http://ayende.com/4689/what-is-wrong-with-this-api#comment40http://ayende.com/4689/what-is-wrong-with-this-api#comment40Fri, 12 Nov 2010 19:21:52 GMTThere 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 http://ayende.com/4689/what-is-wrong-with-this-api#comment39http://ayende.com/4689/what-is-wrong-with-this-api#comment39Fri, 12 Nov 2010 18:38:23 GMTDisencourages ask - don't tell? http://ayende.com/4689/what-is-wrong-with-this-api#comment38http://ayende.com/4689/what-is-wrong-with-this-api#comment38Fri, 12 Nov 2010 18:23:43 GMTWhether 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. http://ayende.com/4689/what-is-wrong-with-this-api#comment37http://ayende.com/4689/what-is-wrong-with-this-api#comment37Fri, 12 Nov 2010 17:22:43 GMTShould return an int or enum for greater flexability http://ayende.com/4689/what-is-wrong-with-this-api#comment36http://ayende.com/4689/what-is-wrong-with-this-api#comment36Fri, 12 Nov 2010 16:11:39 GMTIf there are multiple extensions each with this method, which one do you listen to? http://ayende.com/4689/what-is-wrong-with-this-api#comment35http://ayende.com/4689/what-is-wrong-with-this-api#comment35Fri, 12 Nov 2010 15:26:16 GMTOne 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! http://ayende.com/4689/what-is-wrong-with-this-api#comment34http://ayende.com/4689/what-is-wrong-with-this-api#comment34Fri, 12 Nov 2010 15:07:41 GMTI 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. http://ayende.com/4689/what-is-wrong-with-this-api#comment33http://ayende.com/4689/what-is-wrong-with-this-api#comment33Fri, 12 Nov 2010 15:05:52 GMTMy 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. http://ayende.com/4689/what-is-wrong-with-this-api#comment32http://ayende.com/4689/what-is-wrong-with-this-api#comment32Fri, 12 Nov 2010 14:13:28 GMTThe 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). http://ayende.com/4689/what-is-wrong-with-this-api#comment31http://ayende.com/4689/what-is-wrong-with-this-api#comment31Fri, 12 Nov 2010 13:53:54 GMTShould be a protected method - meant for internal logic, not for the end-user of the class. http://ayende.com/4689/what-is-wrong-with-this-api#comment30http://ayende.com/4689/what-is-wrong-with-this-api#comment30Fri, 12 Nov 2010 13:43:11 GMTThe 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. :) http://ayende.com/4689/what-is-wrong-with-this-api#comment29http://ayende.com/4689/what-is-wrong-with-this-api#comment29Fri, 12 Nov 2010 13:29:10 GMTCallers 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); } } http://ayende.com/4689/what-is-wrong-with-this-api#comment28http://ayende.com/4689/what-is-wrong-with-this-api#comment28Fri, 12 Nov 2010 12:47:22 GMTIt does not allow to specify a reason? http://ayende.com/4689/what-is-wrong-with-this-api#comment27http://ayende.com/4689/what-is-wrong-with-this-api#comment27Fri, 12 Nov 2010 12:41:40 GMTViolates Open/Closed principal? Looks open for modification. http://ayende.com/4689/what-is-wrong-with-this-api#comment26http://ayende.com/4689/what-is-wrong-with-this-api#comment26Fri, 12 Nov 2010 12:36:47 GMTI think the comment does not make sense. http://ayende.com/4689/what-is-wrong-with-this-api#comment25http://ayende.com/4689/what-is-wrong-with-this-api#comment25Fri, 12 Nov 2010 12:29:02 GMT@Koen: Couldn't that be prevented by sealing the class though? http://ayende.com/4689/what-is-wrong-with-this-api#comment24http://ayende.com/4689/what-is-wrong-with-this-api#comment24Fri, 12 Nov 2010 12:05:33 GMTI 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? http://ayende.com/4689/what-is-wrong-with-this-api#comment23http://ayende.com/4689/what-is-wrong-with-this-api#comment23Fri, 12 Nov 2010 12:02:38 GMTVeto-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... http://ayende.com/4689/what-is-wrong-with-this-api#comment22http://ayende.com/4689/what-is-wrong-with-this-api#comment22Fri, 12 Nov 2010 12:02:00 GMTMaybe you are missing the context to base the decision on? public virtual bool VetoMessage(QueueContext context, IncomingMessage message) { return true; } http://ayende.com/4689/what-is-wrong-with-this-api#comment21http://ayende.com/4689/what-is-wrong-with-this-api#comment21Fri, 12 Nov 2010 12:01:42 GMTThere'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). http://ayende.com/4689/what-is-wrong-with-this-api#comment20http://ayende.com/4689/what-is-wrong-with-this-api#comment20Fri, 12 Nov 2010 11:57:19 GMTWe cannot tell by looking at signature what exactly it's return value refers to? http://ayende.com/4689/what-is-wrong-with-this-api#comment19http://ayende.com/4689/what-is-wrong-with-this-api#comment19Fri, 12 Nov 2010 11:53:20 GMTmind 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. http://ayende.com/4689/what-is-wrong-with-this-api#comment18http://ayende.com/4689/what-is-wrong-with-this-api#comment18Fri, 12 Nov 2010 11:52:10 GMTWell 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; } http://ayende.com/4689/what-is-wrong-with-this-api#comment17http://ayende.com/4689/what-is-wrong-with-this-api#comment17Fri, 12 Nov 2010 11:36:43 GMTUsually you will accept the message, so the default should return false and let specialisms do the vetoing. http://ayende.com/4689/what-is-wrong-with-this-api#comment16http://ayende.com/4689/what-is-wrong-with-this-api#comment16Fri, 12 Nov 2010 11:33:19 GMTWhy should one subclass? Interceptor might be a more appropriate solution. http://ayende.com/4689/what-is-wrong-with-this-api#comment15http://ayende.com/4689/what-is-wrong-with-this-api#comment15Fri, 12 Nov 2010 11:30:58 GMT 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 http://ayende.com/4689/what-is-wrong-with-this-api#comment14http://ayende.com/4689/what-is-wrong-with-this-api#comment14Fri, 12 Nov 2010 11:20:03 GMT