Sponsored By Aspose - File Format APIs for .NET

Aspose are the market leader of .NET APIs for file business formats – natively work with DOCX, XLSX, PPT, PDF, MSG, MPP, images formats and many more!

Refactoring guard clauses, or “How to ask politely”

This post is brought to you by the letters M and V and P. I mention that at the beginning to set the pre-requisites.

I woke up this morning, broke open our application, and am currently staring at this method in our presenter.

public void Start( )
{
    if ( !IsAuthorized( ) ) return;
    if ( !ValidateHeritage( ) ) return;

    if ( !IsCousin( ) ) return;
    if ( HasSiblings( ) ) return;
}

My first reaction to this was that it didn’t actually do anything. The way it’s currently set up, it appears to check a bunch of guard clauses and exit if they are met. But there isn’t any actual code being executed if the guard clauses aren’t met.

Digging deeper, I discovered that the guard clauses are actually doing more than guarding. They are calling methods on the View and otherwise changing state. For example, the IsCousin method:

public bool IsCousin( )
{
    if ( _user.AuntsAndUncles.Contains( _prospect.Mother ) 
         || _user.AuntsAndUncles.Contains( _prospect.Father ) )
    {
        _viabilityCount++;
        View.DisableSafetyChecks( );
        return true;
    }

    return false;
}

Clearly, this is ain’t your mother’s guard clause. It’s updating a local variable and doing some fanciness in the UI as well as guarding.

So maybe it’s just an issue with naming. Obviously, it’s not a guard clause, so maybe I could rename it to make that clearer. An obvious name isn’t leaping out but in any case, something feels wrong about having all these methods return a boolean.

An alternative I’m considering is separating the guard clauses from the actions:

public void Start()
{
    if ( IsAuthorized( ) == false )
    {
        SetNotAuthorized( );
        return;
    }
    if ( ValidHeritage( ) == false ) return;

    if ( IsCousin( ) == false ) return;

    if ( HasSiblings( ) == false ) return;
    
    IncreaseViability( );
    DisableSafetyChecks( );
}

Clearly, this is more verbose. But it looks better to me because the guard clauses don’t actually do anything except check the state of something. The actual flow of code is more obvious (isn’t it?).

Maybe the original method is fine and I’ve been reading too much on Command-Query Separation lately. Or maybe there’s an alternative. In which case, I’d love to hear it, even given the very limited context I’ve provided.

And in order to solicit as many opinions as possible, and because I know how much we all love to prove each other wrong, I’ll make the claim that my alternative is THE DEFINITIVE way of doing this.

Kyle the Reversed

This entry was posted in Refactoring. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://kyle.baley.org Kyle Baley

    I will agree that if( false == false ) { then true } is hard to read. But, and I will attribute this to my advancing age, the rest of this comment was a little hard to read too.

    I *think* it’s more valid to say this is hard to read instead: if (false == false) is true then do something. And I will grant that. But if you hold that ! is the same as saying “this is false”, then I don’t see much difference semantically. !IsAuthorized( ) is the same as saying IsFalse( IsAuthorized( ) ) and now you’re only a stone’s throw from false == IsAuthorized.

    In any case, my argument wasn’t whether false == false is hard to read. It was that ! as an operator is easy to miss. And I know that’s true because I’ve missed it. Often. Especially on teams that don’t appreciate the use of whitespace.

    These days, I’m inclined to create negative versions of methods solely to avoid having to use either. So alongside IsAuthorized, I’ll also have IsNotAuthorized. As long as you don’t have any jokers in your crew (e.g. if( !IsNotAuthorized( ) != false ) { } else { DoSomething( )} ), it’s made code reviews easier on me.

  • http://pulse.yahoo.com/_I7RC7AMV2L5LT6GNZJORIA3I44 TSmith

    Disagree with the use of 
    “If(IsAuthorized( ) == false( { DoSomething(); }”. 
    You are now reading a false literal in order to determine if the condition is true…eh? say that again. Yah, you want the condition to be false in order to be true for the if..then logic. That feels wrong, looks weird and is hard to read for me, personally.  if(false==false){ then true} )…that’s just silly

  • Pingback: Keeping Your Ruby Smelling Fresh ‘N Clean « charlesluzar

  • http://colinjack.lostechies.com Colin Jack

    I’d definitely vote for refactoring here, I also agree on preferring “== false” instead of “!”.

  • Steve Py

    It’s an improvement to avoid having methods do more than one thing. I definitely favor “1-way-in-1-way-out” constructs when addressing behaviour logic.

    public void Start()
    {
    if ( !IsAuthorized( ) )
    {
    SetNotAuthorized( );
    }
    else if( ValidHeritage( ) && IsCousin( ) && HasSiblings( ) )
    {
    IncreaseViability( );
    DisableSafetyChecks( );
    }
    }

    Guards would just return a default indicator, or throw exceptions on rule violations, not govern over behaviour.

    Personally I’d probably have had IsAuthorized as a guard that raised a NotAuthorizedException() [Custom Exception] as it would likely be a fairly common guard check and would be handled consistently at a lower level of the application rather than being implemented in each presenter/view.

    if ( !IsAuthorized( ) )
    throw new NotAuthorizedException(string.Format(” Unauthorized access to {0}”, _view.Name ) );

    if( ValidHeritage( ) && IsCousin( ) && HasSiblings( ) )
    {
    IncreaseViability( );
    DisableSafetyChecks( );
    }

  • http://jamespeckham.com James Peckham

    And i agree with thomas about the redundant == comparison but could see how that might be a team preference with someone who has a special need.

  • http://jamespeckham.com James Peckham

    I like your post, it reminds me of “Clean code” by robert martin.

    He says something like “A method works best when it does just one thing.”

    If you have a hard time naming a method it’s usually a good sign that it does more than one thing :)

    well it does this… AND it does this… AnD it does this.

    Bad sign. Split those things up :)

  • http://codebetter.com/members/kylebaley/default.aspx Kyle Baley

    Thomas: Strictly personal preference. My eyes aren’t what they used to be and having a string of conditionals (as in my first example) makes it easy for me to skip the ! operator, or assume one’s there when it isn’t. It’s not something I’ll religiously adhere to but if I have full control over the code, I’ll use it when I can.

  • http://clearer-code.com Thomas Johnson

    It seems quite clear to me that methods with names like IsCousin shouldn’t change state. So I definitely agree that your refactoring is an improvement.

    Is there a reason you use ” == false” rather than the not operator “!” e.g.

    if ( !ValidHeritage( ) ) return;

    Cool Captcha gadget.

  • Razvan Dimescu

    I would have done the same if I were you. Clearly the first method is lying.

  • Jason Meckley

    I would consider this a problem. the members are asking for state. At the same time the members are modifying the state. CQS says that asking for the current state should not modify the object. For example, what would happen if you called IsAuthorized() twice?

    I would consider your refactoring verbose. It explicitly states what is happening. If anything the original code was insufficient, not concise.