Violations Of The “Tell, Don’t Ask” and “Don’t Repeat Yourself” Principles?

In the last few years, I’ve written a lot of code that looks like this:

   1: public class IsThisATellDontAskViolation

   2: {

   3:   public void DoBadThings()

   4:   {

   5:     if (something.CanHandle(anotherThing))

   6:     {

   7:       var response = something.GetThatFromIt(anotherThing);

   8:       DoSomethingWithTheResponse(response);

   9:     }

  10:     else

  11:     {

  12:       DoSomethingElse();

  13:     }

  14:   }

  15: }

Look at lines 5 and 7, specifically. In this code, I’ve got a call being made to a dependency to check whether or not the dependency can handle whatever it is I want to send to it. If the dependency can handle it, I call another method on the same object, passing in the same parameter and expecting to be given a result of the processing.

This code bothers me, honestly, but I find myself constantly writing it. It appears to me, to be a violation of both the Tell, Don’t Ask and Don’t Repeat Yourself (DRY) principles. I don’t like how the class questions the dependency and then has it do something based on the response. This seems like a violation of Tell, Don’t Ask and is clearly falling back to simple procedural programming techniques. Then, having the same “anotherThing” variable passed into another method on the same dependency object seems like its violating DRY. Why should I have to pass the same thing to the same object twice?

Another example of this type of code can be found in my SOLID Principles presentation and sample code. In the Open Closed Principle, I create the following code:

   1: public string GetMessageBody(string fileContents)

   2: {

   3:   string messageBody = string.Empty;

   4:   foreach(IFileFormatReader formatReader in _formatReaders)

   5:   {

   6:     if (formatReader.CanHandle(fileContents))

   7:     {

   8:       messageBody = formatReader.GetMessageBody(fileContents);

   9:     }

  10:   }

  11:   return messageBody;

  12: }

You can clearly see the same pattern with the CanHandle method on like 6 and the GetMessageBody method on line 8. For the same reasons, this code bothers me. It always has, but I’ve never done anything to correct it.

 

Is This A Violation Of Tell, Don’t Ask And/Or DRY?

Ultimately, that’s what I’m asking… are these code samples violating those principles? … and Why or Why Not? I would love to hear your opinion in the comments here or as your own blog post.

 

One Possible Solution

Assuming that this is a violation of those principles (and I’m all ears, listening for reasons why it is or is not), I have one solution that I’ve used a number of times for a similar scenario. Rather than having a method to check if the dependency can handle the data sent to it, just have one method that either processes the data or doesn’t, but tells you whether or not it did through the returned object.

   1: public class Response<T>

   2: {

   3:   public bool RequestWasHandled { get; private set; }

   4:   public T Data {get; private set;}

   5:   public Response(bool requestWasHandled, T data)

   6:   {

   7:     RequestWasHandled = requestWasHandled;

   8:     Data = data;

   9:   }

  10: }

  11:  

  12: public class TellDontAskCorrection

  13: {

  14:   public void DoGoodThings()

  15:   {

  16:     var response = something.GetThatFromIt(anotherThing);

  17:     if (response.RequestWasHandled)

  18:       DoSomethingWithTheResponse(response.Data);

  19:     else

  20:       DoSomethingElse();

  21:   }

  22: }

This works. I’ve used it a number of times in a number of different scenarios and it has helped clean up some ugly code in some places. There is still an if-then statement in the DoGoodThings method but that may not be avoidable and may not be an issue since the method calls in the two if-then parts are very different. I don’t think this is the solution to the problems, though. It’s only one possible solution that works in a few specific contexts.

 

Looking For Other Solutions

What are some of the patterns, practices and modeling techniques that you are using to prevent Tell, Don’t Ask and DRY violations? Not just for the scenario that I’ve shown here, but for any given scenario where you find yourself wanting to introduce procedural logic that should be encapsulated into the object being called. I would love to see examples of the code you are dealing with and the solutions to that scenario. Please share – here in the comments (please use Pastie or Github Gist or something else that formats the code nicely if you have more than a couple lines of code, though) or in your own blog, etc.


Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Derick Bailey

Derick Bailey is an entrepreneur, problem solver (and creator? :P ), software developer, screecaster, writer, blogger, speaker and technology leader in central Texas (north of Austin). He runs SignalLeaf.com - the amazingly awesome podcast audio hosting service that everyone should be using, and WatchMeCode.net where he throws down the JavaScript gauntlets to get you up to speed. He has been a professional software developer since the late 90's, and has been writing code since the late 80's. Find me on twitter: @derickbailey, @mutedsolutions, @backbonejsclass Find me on the web: SignalLeaf, WatchMeCode, Kendo UI blog, MarionetteJS, My Github profile, On Google+.
This entry was posted in .NET, Analysis and Design, AntiPatterns, C#, Principles and Patterns. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.lostechies.com/members/gnschenker/default.aspx Gabriel N. Schenker

    If the method would be an action with no response then I agree with you it can be a violation of the tell don’t ask principle and of DRY; but even then there exist scenarios where it still makes sense to have the two methods (the one that asks “can you do that” and the other that “does it”). A good example is the strategy pattern where you have a set of strategies and you only want to execute the first (and not all!) strategy that can handle the request…
    But in the case where you have a function with a response it is even more questionable whether you should try to “fix” you “ugly” code. It really depends on the scenario you are dealing with. One possible alternative solution is the way how the TryParse functions are implemented for the data types in .NET, e.g. int.TryParse(value, out result). But I personally do not like this syntax since there is an out parameter involved…
    Consequently my answer to your question is: … it depends…

  • Robert

    The TryParse family of methods are a diffrent way of handeling this that avoids the return type wrapper.
    the method returns True/False based on success and has an “out” parameter to capture the value.

    public class TellDontAskCorrectionTryParse
    {
    public void DoGoodThings()
    {
    T response;
    if (something.TryGetThatFromIt(anotherThing, out response))
    DoSomethingWithTheResponse(response);
    else
    DoSomethingElse();

    }
    }

  • perryneal@hotmail.com

    If formatReader.GetMessageBody() was constructed in such a way that it returned an empty string or NULL if it couldn’t handle the file contents, then it would be the client’s responsibility to check.

  • http://jonfuller.codingtomusic.com Jon Fuller

    I’ve usually end up at the same place you did (with your Generic Response class). I’ll normally toss an implicit cast on it for the bool case too, just to slim down the calling code a little more. Sometimes I even put a helper ‘If’ function on that class so I don’t have to store the response value in a variable. Check out this gist for both the implicit cast and If function: http://gist.github.com/415778

    I recently realized that this is a paradigm the functional folks have been using for a long time called option. (http://msdn.microsoft.com/en-us/library/dd233245.aspx)

    The other thing I’ve seen that fits in this category, that I’m not particularly in love with is the TryParse idiom with the bool return value, and an out parameter to give you your value back. It’s at least another option, but like I said, not one that I like.

  • http://derekgreer.lostechies.com Derek Greer

    Technically, this would be a violation of Tell, Don’t Ask since you’re using information derived from the object to make decisions about how the object is to be used.

    In this case, rather than moving the retrieval of the state and the operation upon the state into the object, I’d probably eliminate participation of the object in the question altogether. Is it really the responsibility of any class to provide information about whether it can handle the parameters you pass to it?

    The way I’ve handled this in the past is to have a class whose sole responsibility is dispatching. The FileReaderService fits this bill partially, but it relies upon the registered IFormatReaders to tell it which one to use rather than keeping track of this information itself. If you register the reader along with a predicate then each IFormatReader would only be responsible for reading, not for participating in the dispatching process. For example:

    fileReaderService.RegisterFormatReader(new XmlFormatReader(), new XmlContentHandlerPredicate());

  • http://www.trainedchimpanzees.blogspot.com/ Lorenzo Jorquera

    I think the need of this kind of code indicates a code smell call Refused Bequest ( http://sourcemaking.com/refactoring/refused-bequest)

    The problem is that some objects can handle the request and others with the same interface can’t. What would happen if you call GetThatFromIt in an object that returns false in CanHandle? I supose that an exception would be thrown. But then something is wrong with the hierarchy, as is a little extrange that you implement some interface but do not really implement all its methods.

  • http://sharpbites.blogspot.com alberto

    It smells to me as a LSP violation, as Lorenzo pointed out.

  • http://blog.sampy.com Sampy

    I use the Response pattern in Channel 9 for calls to my services. Mine is called CommandResult (there’s also a non-generic version for commands that don’t have a return value) and it has Sucessful, ShouldRetry, and Errors as addtional properties.

    The biggest thing that it has allowed me to do is create a helper method InvokeCommand that takes lambdas to 1) call the service, 2) what to do if successful, 3) what to do on failure, and 4) what do to on fail but retry.

    I was worried that by not throwing an exception or something else unignorable that errors could get lost but with the helper method to invoke services I get clean controllers and better assurance that I’m not missing something.

  • http://helpqlodhelp.blogspot.com Ralf Bokelberg

    I like Derek’s explanation & solution very much.
    I used code like you have in your first example to implement a message parser framework, which can be extended at runtime. New parsers were added to a dispatcher in Spring. The dispatcher uses the canHandle/handle methods to find and execute the parser for a given input. The idea behind it, is to encapsulate the understanding of the message format in the parser. With Derek’s solution it is still nicely encapsulated.

  • http://www.numainnovations.com/mentis-vulgaris Jason Smith

    Without knowing more about your situation, it appears GOF Chain Of Responsibility pattern may address this.

    – Jason

  • http://weblogs.asp.net/dotjosh Josh Schwartzberg

    I hate being a pattern zealot, and this is overkill for your concrete example. But the “Chain of Responsibility” pattern fits in nicely with what you are wanting to accomplish.

  • http://weblogs.asp.net/dotjosh Josh Schwartzberg

    PLEASE DELETE MY LAST COMMENT, ITS HARD TO READ THE CODE:

    And with that, here is a quick and dirty example that’s in a formatted link http://gist.github.com/416324

  • Brandon Behrens
  • Brandon Behrens

    At the very least, the decision about whether or not to have IsThisATellDontAskViolation actually handle the request should be in a different method.

  • http://twitter.com/VitalyStakhov Vitaly Stakhov

    As for me this is definitely Tell don’t Ask violation as we’re making decisions about asking an object to do something for us based on its internal state.
    I don’t see this little DRY violation as a problem yet, however it will become stronger if other clients of that ‘class with CanHandle/Handle methods’ will appear.

    As a possible option for the first example would be to use doulbe dispatch (where ‘something’ will call DoSomethingWithRespose and DoSomethingElse methods). But it really depends on the concrete situation if it’s applicable.

    For format readers, I agree with the commenters above suggesting the Chain of Responsibility pattern.

  • http://www.lostechies.com/members/jagregory/default.aspx James Gregory

    For those people suggesting the Chain of Responsibility: “Do not use Chain of Responsibility when each request is only handled by one handler, or, when the client object knows which service object should handle the request.” -c2 wiki *

    Derick’s code implies that a request will only be handled by one handler,as the messageBody variable is overwritten rather than appended to.

    Annoyingly though, the c2 wiki doesn’t suggest alternatives for this situation.

    * http://www.c2.com/cgi/wiki?ChainOfResponsibilityPattern

  • Bill Christie

    Here is an alternative that is relatively simple to implement and can work well in certain situations.

    public class TellDonTAskAlternative
    {
    public void DoGoodThings()
    {
    var response something.GetThatFromIt(anotherThing, () => FallbackAction());
    }
    }

  • Aaron Ransley

    @Bill Christie: I like your solution the most. Would lend the tightest syntax, so long as the FallbackAction wasn’t too involved.

  • http://flatlinerdoa.spaces.live.com Andrew Chisholm

    Response seems similar to the Maybe monad. I’ve used this in some C# projects which results in cleaner code and the ability to daisy chain methods together (fluent style).

    See http://abdullin.com/journal/2009/10/6/zen-development-practices-c-maybe-monad.html or http://weblogs.asp.net/zowens/archive/2009/09/04/maybe-monad-my-c-version.aspx for examples

  • http://codebetter.com/blogs/ian_cooper/ Ian Cooper

    To remove an if statement polymorphism is your friend. Generally the model is something like this:

    1: Ask factory to give you a strategy for your criteria
    2: Exercise strategy

    So in pseudo-code:

    1: public string GetMessageBody(string fileContents)

    2: {

    3: 4: foreach(IFileFormatReader formatReader in _formatReaders)

    5: {

    6: var messageReader = new MessageReaderFactory(fileContents)
    7: messageBody += messageReader .GetMessageBody();

    8: return messageBody;

    9: }

    Where a messageReader than cannot process the file contents simply returns an empty string. This latter part is really just a variation of the null object pattern.

  • Henning Anderssen

    Why not use a event system, such as Udi Dahan’s domain events.

    http://www.udidahan.com/2009/06/14/domain-events-salvation/

    It the GetThatFromIt passes, you send a passed event, else you send a failed event.
    Then you can chain events that does one thing. Requires a bit more infrastructure in place, but I think events can solve a lot of problems :)

  • Peter

    Hi!
    Why not do the following..
    public class handler {
    IRequest _theRequest;
    public bool can_handle(IRequest someRequest) {
    /// figure out if it can handle then store someRequest locally.
    // if you cannot handle this… return false .. else…
    _theRequest = someRequest;
    return true;
    }

    public void Handle(){ /*does what it needs to… */ }
    public IResult Handle() { /*does what it needs to but returns a status result… */ }

    }

    public class consumerClass {

    public void some_method() {

    foreach ( var item_to_test in some_collection_of_handlers ){
    if ( item_to_test.can_handle(some_request) {
    item_to_test.Handle();
    }
    }

    }
    }

  • crushcrush

    I think the idea is to encapsulate the “CanHandle()” logic inside of the GetMessageBody() function. If CanHandle() would be false, then GetMessageBody() would simply return an empty string.

  • deltreme

    Anyone would use the same pattern for files:

    if (File.Exists(path))
    File.Open(path);

    As you should depend on the exception File.Open() would otherwise throw. I quite like the TryXxx alternatives from .NET:

    int number;
    if (int.TryParse(str, out number))

  • Trystan Spangler

    Another possible solution is continuation passing style. Make your Handle method take an Action to use if it can handle the input and an Action for when it can’t handle it. Sort of like success and failure callbacks. I can’t say if it’s better or not – but seems very Tell Don’t Ask.