Advertisements
Posted by: jsonmez | June 28, 2010

Do We Need If Blocks?

I’ve been contemplating this for a while now.  I thought I would finally write on the subject and see what other people thought.

Do you really need blocks after if statements?

It has always been a best practice, in my opinion, to put opening and closing brackets after an if statement.  Even if it has only one statement following.

if(theSkyIsBlue)
{
   DanceALittleDance();
}

The reasoning behind this best practice goes something like this: If someone later modifies this code and adds a line inside the if, having the block will prevent them from doing something like this:

if(theSkyIsBlue)
   DanceALittleDance();
   MakeALittleLove();

Which, of course, will result in way too much love making, even on stormy days when the sky is mostly gray.

All those { } add up to a lot of overhead

I’m not sure it is worth it anymore.

Most of the code I write now has one statement after an if block.

If I have some code like:

if(theSkyIsBlue)
{
   PutYourRightFootIn();
   TakeYourRightFootOut();
   DoTheHokeyPokey();
   SpinYourselfAbout();
}

It is pretty much always going to get refactored to:

if(theSkyIsBlue)
{
   DanceALittleDance();
}

So what is the real value of the { } here?  If someone modifies this code, they are most likely going to modify the method DanceALittleDance, they really shouldn’t be adding anything to the if block.  Even if they do, I think I’m paying a pretty high curly bracket tax all over my code to prevent someone from doing something stupid later on.

Is this really that bad?

if(theSkyIsBlue)
   DanceALittleDance();

Perhaps there is a better best practice?

How about if we forgo all the extra curly braces and instead say:

Whenever possible replace an if block with a single statement.

Hmm, I think I like that better.  We won’t run the risk anymore of accidentally adding a statement after the if block, if we are used to thinking of an if block itself as a code smell.

Almost always you can describe what is being done in an if block with a better name and encapsulate it in a method.

Of course there are exceptions here.  In a method with a void return type when you early return in an if block, you can’t really replace that with a single statement.  (Although sometimes you can avoid the early return by using an if else statement instead.)

We can apply the same thinking to anything that has a block statement.

Strangely though, try and catch require block statements to follow.  You cannot have a single statement following a try.

Anyway, at this point I am pretty well convinced that including the { } just because is a pretty archaic practice and is unnecessarily cluttering up our code.  If I am wrong, tell me why I am wrong, but as far as I can tell, if we prefer to extract statement blocks into methods, we can avoid the curly bracket tax everywhere.

As always, you can subscribe to this RSS feed to follow my posts on Making the Complex Simple.  Feel free to check out ElegantCode.com where I post about the topic of writing elegant code about once a week.  Also, you can follow me on twitter here.

Advertisements

Responses

  1. I’m not sure: what is the source of your overwhelming hatred of curly brackets. Are you just objectionable to the number of lines they take up? If so, why not just write it as:

    if(theskyisblue){DanceaLittleDance();}

    Also, how was it you were expecting to purge the if from your code? I mean, I suppose you could pass your variable to the method for evaluation, but your method is just going to end up checking it with an if before it does anything.

    • Well, it is not just the lines. Is the idea of doing something that no longer makes sense. Seems like a big waste to throw those curly brackets in every time, when the value it provides is less and less.

      I’ve always put the { } after every if block, even when it had only one line, and never really questioned it before. But I find as I am questioning it now, I don’t really have a great reason to keep doing it.

      For me it is coming down to this: A better best practice that will make the code more clear and reduce errors is to encourage encapsulating logic that would be in an if block in a method call rather than trying to a { } to prevent someone from accidentally adding a line that isn’t part of the if condition.

      I realize we could do both, but out of the two “best practices” which would end up in better code with less defects overall?

  2. So, what you’re really saying is that you want to switch to VB.net which requires neither {}s nor ;s. Come to the dark side (well, for Windows machines anyway. I suppose your “alternate” languages will do for other operating systems).

    • I’m not quite advocating that.

      The only difference between C# and VB in that respect is the replacement of { } with BEGIN and END. If you have an if block in VB.NET you will still need BEGIN END type syntax.

      Interesting to think about the use of the ; though. When I program in VB.NET I always accidentally put it, and I curse VB.NET for not having semi-colons.

      But, if you think about it. Do we really need it in C# or Java or would a simple newline suffice? Is it that we just think it should be there because that is what we feel comfortable with?

      • Yes, the ; is my bane. I detest it with all my heart. Why not just let a line break indicate a programming line break. Honestly, what percentage of the time does your line of code wrap down to the next line? For me it’s somewhere in the neighborhood of 1 to 5%. I would much rather have to explicitly tell it that I am continuing my line in these exceptional cases than tell it I am not cintinuing my line for the other 95-99% of the time.

        I think I saw a snippet when they were previewing Visual Studio 2010 that C# was going to be ; optional. I don’t have a copy of it here to confirm, but wouldn’t that be a dandy reduction in unwanted characters.

      • @Chris I find it hard to believe your lines wrap only 1-5% of the time. Do you use object initializers, LINQ, or any fluent APIs? I find my code wrapping much more like 50% of the time. If C# required a character like _ to signal line wrapping it would make reformatting code very painful.

        @John Get Resharper, setup whatever code formatting rules you want and then use it to auto-format your code. It’s bliss!

  3. I totally disagree. Have seen way to many times where there are no brackets and someone adds a statement and thinks it is part of the if statement. When in doubt, put brackets. Makes it way more readable.

    • I know this is a religious topic, and up to now I have been on the other side of it.

      But I have to ask, is it really more readable?
      And most of our IDE’s automatically handling indenting now, so if you type a line below and if it will look like:

      if(blah)
      DoBlah();
      Ooops();

  4. Like you I was always on the other side saying that all statements needed to have braces. I agree that for one line statements the braces just add noise. There are a lot of little things that us programmers do that we initially think will help readability, but end up just causing extra noise that needs to be maintained.

    One of that standards that I’d like to see removed from my current work place is adding #region and #endregion statements that surround private, public, protected, internal, constructor, properties, etc code groupings. This standard was created because our code files were too large. This allowed the programmer to find the information that they wanted by collapsing the regions in the IDE.

    Now they are nothing but noise and extra lines. They even have to be maintained because if you add a #region without an #endregion you can’t compile.

    I think that any noise that can be eliminated should be eliminated. I even agree with @Steve that the semicolon is noise. Even comments to a certain degree are purely noise. I Uncle Bob makes a very good argument against most comments in his “Clean Code” book.

    • Sorry, @Chris not @Steve in my last message…

    • Agree with you 100%. I can’t stand regions. When it first came out, I thought it was great, but all it does is hide a mess that you should deal with in another way.
      Regions are basically like shoving a bunch of crap in different closets around your house to make it look clean.

      Clean code is an excellent book, take a look at my post on comments if you haven’t seen it. Amazing how many people disagree.
      http://elegantcode.com/2010/04/18/eliminating-comments-the-road-to-clarity/

  5. I have recently moved from VB to Objective-C, and decided that for single line IF blocks I would just code them inline…

    if (theSkyIsBlue) DanceALittleDance();

    This reduces the { clutter, but also helps to avoid the possbility that if a second statement needs to be added to the IF block, it won’t just get added as another indented line after the existing statement. (Causing the logic problem you raise.)

    This approach also works well if you have several one liner IF blocks in a row. (Not that that happens a lot, but it does occasionally.)

    • I actually like your solution the for a few reasons

      1. The elimination of the problem of accidentally thinking something is going to be in the if block, because it is either on a block or on the same line.

      2. It reads much more clearer, if this then that all on the same line like a sentence.

      3. It forces you to have both a short condition and statement. Which is going to encourage putting large conditions into human readable methods.

      You have convinced me. I can see no draw-back to your method of handling if statements.

      Thanks Bill!

  6. […] Do We Need If Blocks? (John Sonmez) […]

  7. I agree with the first part of your Post and with Bill, but I strongly disagree with your statement:
    Whenever possible replace an if block with a single statement.

    I would change it to:
    Replace an if block with a single statement if this makes the code more readable.

    I would argue that in many cases it doesn’t.
    For example, I would consider this code more readable:

    if(someCondition)
    {
    Post post=posts.First(p => p.Id == id);
    post.MarkAsRead();
    post.ShowInInbox = false;
    }

    than:

    if(someCondition) MarkAsReadAndDontShowInInbox();

    When reading the first block I can see with one glance what is going on, instead of having to look up the “MarkAsReadAndDontShowInInbox” method.

    • Hmm, although you present a very good example there, I’m not 100% convinced.

      If the method where that work is happening contains some other logic, even the small if block can be a little noisy.
      Consider also that if you create the method MarkAsReadAndDontShowInInBox(), you are abstracting away how you mark something read and not show it in the inbox from what you are doing. This has some value, because you can change that implementation detail without changing the rest of the method it is contained in.

      In your example there the code is small and clear, but even though it is a tiny difference, I would still say it is easier to read MarkAsReadAndDontShowInInbox than 3 lines of code.

      Consider also the larger scale effect of having most of your if blocks replaced with a single statement. The readability of one particular block might be negligible difference, but throughout your entire code-base, I think you would see a significant readability improvement.

      • I’ve even seen this suggestion. I believe both Martin Fowler and Robert Martin talk about in their books. I think this is taking it to the _extreme_ and I would rarely use this, but I wanted to throw it out there as another example.

        Instead of:

        if(someCondition) MarkAsReadAndDontShowInInbox();

        Break it up:

        if( someCondition ) MarkAsRead( id );
        if( someCondition ) RemoveFromInbox( id );

        This seems counterintuitive because you duplicating logic. However it is a lot easier to reuse the MarkAsRead and RemoveFromInbox functions from other parts of code and I’ve seen this lead to better refactoring ideas.

  8. I always use {} except for the case where there can be no more execution afterwards, i.e. when the if is followed by a return or throw.

    Keeping the aforementioned goodness of {} and keeping the code clutter free, consider guard clauses like so:

    if(theSkyIsBlue)
    {
    PutYourRightFootIn();
    TakeYourRightFootOut();
    DoTheHokeyPokey();
    SpinYourselfAbout();
    }

    becomes

    if(!theSkyIsBlue)
    return;

    PutYourRightFootIn();
    TakeYourRightFootOut();
    DoTheHokeyPokey();
    SpinYourselfAbout();

    I think that reading pre-conditions like that in the beginning improves the readability.

    Furthermore, although we use { on a new line at work (C# shop), as a JavaScript hobbyist, I’ve switched to { on the same line which saves me one line and reads better IMHO.

  9. I don’t necessarily agree with you (I agree on severely limiting the use of the IF statement), but I did find this recently and figured you might sign up in support or at least find it amusing):

    http://www.antiifcampaign.com/

  10. I have been thinking about this myself just lately, trying to decide the cost of these curly braces.

    I came to this conclusion.

    If the code exhibits high readability, why the concern?

    I would encapsulate this, as a reusable pattern, after all it is the hokey cokey, and I will use it lots:

    PutYourRightFootIn();
    TakeYourRightFootOut();
    DoTheHokeyPokey();
    SpinYourselfAbout();

    But I agree with Heiko I would not encapsulate this:

    if(someCondition)
    {
    Post post=posts.First(p => p.Id == id);
    post.MarkAsRead();
    post.ShowInInbox = false;
    }

    It reads great, and makes perfect sense, with the assumption that someCondtion has a name with contextual meaning.
    More importantly, it is instantly accessible, I dont have to follow a chain of method calls to get to the simple code.

    If it is not broken dont fix it, how far can we abstract away our abstractions, the code has to go somewhere.

    Will this section of code be used more than once?
    Is the code low level, hard to understand?
    Is my method already 270 lines of code?

    if so break it up / encapsulate it?

    Sometimes, and usually, if I find I am writing a curley braced code block, it is usually acting on the values of variables set in the current methods scope.

    if(youCantReadCode)
    {
    wontUnderstandThis = previouslyStatedUnderstanding.Value;
    getConfusedByTheBraces = sameAsValuePassedToThisMethod;
    confusionLevel = someObjectInThisScope.Value;

    }

    if we encapsualte that, we have to pass params as well.

    if(youCantReadCode)CodeReview(previouslyStatedUnderstanding.Value, sameAsValuePassedToThisMethod, someObjectInThisScope.Value);

    That to me looks more confusing than seeing the code in the if block, so

    If the number of lines of code to be encapsulated is equal to or less than the number of params required to pass into the encapsualted methods to perform processing, then why bother, its just an extra barrier in getting to the code.

    BUt, hey, I dont in fact know.

    I just found this interesting and thought I would share.

    🙂

    • I see your points here, but there are a few thing I would disagree with or prefer a better solution.

      If a section of code is going to be used more than one, certainly make it a method, but just because it isn’t doesn’t mean you shouldn’t.
      I would consider “Will this code be self-documenting by being extracted into a method with a good name?”

      Also I know the problem you are talking about with having to pass parameters to a method when you extract a method during a refactor.
      In my opinion, the proper approach is to go ahead and refactor and then eliminate the parameters by making them members of the object.
      If it doesn’t make sense to make them members of the object, you have a class that has more than one responsibility, so break it up.
      But, in most cases you can refactor a method out of another method in the class, and find that the 3 parameters it takes can not be refactored into member level variables, which makes everything cleaner and increases the cohesiveness of your class.

  11. MarkAsReadAndDontShowInInbox() breaks the golden rule of OOP or Robert Martin’s Clean code which is “do only one thing”.

    i prefer jerods –
    if( someCondition ) MarkAsRead( id );
    if( someCondition ) RemoveFromInbox( id );

    • Hmm good point. I agree with you.


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Categories

%d bloggers like this: