30

I've inherited some code that makes occasional use of the following if notation:

if (a)
    foo();
{
    if (b)
        boo();
    moo();
}

I'm not sure how to read that naturally but the code compiles and runs.

Can someone please explain how this works so that I can rewrite it in more human readable format? Alternatively, could someone explain why this notation could be useful?

Bernhard Barker
  • 54,589
  • 14
  • 104
  • 138
Simon
  • 855
  • 9
  • 24
  • 28
    `if (a) foo(); if (b) boo(); moo();` – L.B Aug 01 '17 at 22:33
  • 12
    As to your last sentence - I can't think of *any* reason this notation could be useful. – BJ Myers Aug 01 '17 at 22:36
  • 4
    are you sure you are not forgetting an `else` from the code you inherited? – Mike Nakis Aug 01 '17 at 22:36
  • 25
    The syntax is perfectly valid, but the intentions are not. – Brian Rasmussen Aug 01 '17 at 22:50
  • 17
    It's worth inspecting the version control history of this section of code to see if an `if` or `else` was present at some point. You might even get a commit message indicating why it changed. – Russell Borogove Aug 02 '17 at 02:29
  • 4
    I second on checking the history. If I encountered code like that, I would immediately assume that foo() was put in the wrong line by mistake and it's a bug. Of course, the real code might have enough hints (and/or knowing the domain area) to dispel that assumption - but it looks very wrong. – Euro Micelli Aug 02 '17 at 04:54
  • 1
    I have seen `{}` used to make a colapsable group in visual studio. For no other reason than 'readability' without using region. – Drag and Drop Aug 02 '17 at 09:37
  • If you know what if-statements' syntax looks like and keep in mind that C# doesn't care about indentation, you'd have a different (much better and more appropriate) question. – Bernhard Barker Aug 02 '17 at 13:03
  • 1
    I changed the title to something more explanatory. I can't really think of a better way to describe this than "an unconnected block". – Bernhard Barker Aug 02 '17 at 16:15

2 Answers2

69

The code you've posted would be better written as:

   if (a)
   {
       foo();
   }
   if (b)
   {
       boo();
   }
   moo();

Braces in C# have two purposes:

  1. They create a scope for variable declarations.
  2. They group statements together so that conditionals and loops and such can apply to several statements at a time.

Whoever wrote the code you've posted chose not to use them for the second purpose. if statements can be totally legitimate without using any braces, but they'll only apply to the statement that immediately follows them (like the call to foo() after the first if).

Because there is a legitimate use case for braces that has nothing to do with control flow, however, it is perfectly acceptable for someone to put braces in random places that have nothing to do with the if statements.

This code:

foo();
{
   var a = boo();
}
{
   var a = moo();
}

... is equivalent to this code:

foo();
var a = boo();
var b = moo();

... but you'll notice that I couldn't name the second variable a because it's no longer separated from the first variable by scoping braces.

Alternatively, could someone explain why this notation could be useful?

There are three possibilities I can think of:

  1. They wanted to reuse variable names in different parts of the method, so they created a new variable scope with the braces.
  2. They thought braces might be a nice visual aid to encapsulate all the logic that's found inside them.
  3. They were actually confused, and the program doesn't work the way they think it does.

The first two possibilities assume they're actually doing more than calling foo() and moo() in the real production code.

In any case, I don't think any of these possibilities are good reasons to write code like this, and you are totally within your rights to want to rewrite the code in a way that's easier to understand.

StriplingWarrior
  • 151,543
  • 27
  • 246
  • 315
  • 19
    or because there used to be an if statement and somebody removed it but didnt remove the braces (maybe they had it commented out, then removed the commented out line) – pm100 Aug 02 '17 at 01:28
  • 4
    I don't think those last two code snippets are equivalent, the first doesn't leaves neither `a` nor `b` in scope, the second one leaves both. The actual equivalent code would be `foo(); boo(); moo();`. – Todd Sewell Aug 02 '17 at 08:26
  • there is a 4th option why one might do this, reducing the memory footprint. this is especially useful in recursive methods to preserve both stack and heap space since the variables are only available for garbage collection once they are out of scope with no references remaining. But then again, if this is an issue there is probably something wrong with the structure of your code. – prof1990 Aug 02 '17 at 09:25
  • Excellent answer. I actually recently re-read the fun article by joel-spolsky himself on that topic: [Making wrong code look wrong](https://www.joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong/), it also addresses this kind of scenario. I'd consider this a code smell and second rewriting it. – Gero Aug 02 '17 at 10:41
  • 1
    For an example of what @pm100 talks about, read the question of the thread [Explicit Local Scopes - Any True Benefit?](https://stackoverflow.com/questions/16871395/) That thread can also be a help if you are unfamiliar with so-called explicit local scopes, i.e. the "unmotivated" use of `{ ... }` inside the body of a method (accessor, constructor, etc.). – Jeppe Stig Nielsen Aug 02 '17 at 11:13
  • @pm100 I find it extremely unlikely that you'd successfully reduce the memory footprint of a C# application that way. I have it [on good authority](https://stackoverflow.com/questions/7383016/reference-type-variable-recycling-is-a-new-reference-variable-created-every-lo/7383090#comment8915232_7383090) that the compiler can typically figure out where you stop using a given variable, regardless of where you declare it. The accepted answer in the post that Jeppe mentions agrees. So variable scoping is really just to help humans who have to read the code. – StriplingWarrior Aug 02 '17 at 15:23
  • @StriplingWarrior - i think you mean prof1990, i never said anything about memory – pm100 Aug 02 '17 at 17:00
  • Yes, sorry, I meant @prof1990. Sorry about the confusion. – StriplingWarrior Aug 02 '17 at 18:01
1

The curly brace starts after foo() instead of if(a) (like if(a){) therefore these braces are useless in this context.

if (a)
    foo();
//{
    if (b)
        boo();
    moo();
//}

which is equal to

if (a)
    foo();

if (b)
    boo();

moo();

Braces like those are used to limit context. Variables created inside these braces will exist only until the brace ends.

SMUsamaShah
  • 7,677
  • 22
  • 88
  • 131