Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have used multiple lines of code in one if condition without using curly braces, which is executing properly. So what is the main necessity of using curly braces in if statements for multiple lines?

 if (ggcCompareGrid.TopLevelTable != null)
            foreach (GridRow row in ggcCompareGrid.TopLevelTable.Rows)
            {
                if (row.DisplayElement.ParentGroup != null)
                    if (row.DisplayElement.ParentGroup.Category != null)
                        if ((int)row.DisplayElement.ParentGroup.Category == key && row.Record == null)
                        {
                            row.DisplayElement.ParentGroup.IsExpanded = false;
                            break;
                        }
            }
share|improve this question
4  
Have you researched this all through SE? This question (and fairly large list of related cousins) is old enough to have whiskers on it. programmers.stackexchange.com/questions/16528/…. - stackoverflow.com/questions/2125066/… In your case, somebody not clear about the indents/structure (which can happen if function too long to read on a single screen) can easily mess up by adding a line after the foreach at the same indent-level. Also, with 4-5 indent levels,consider refactoring. –  shivsky Dec 18 '13 at 12:15
5  
This question appears to be off-topic because it is about a basic coding practice. The question could essentially be reworded as, "Why should I indicate the beginning and end of code blocks when it isn't required?" Which would be language agnostic, still answer the OPs question, and isn't really Code Review. –  Daniel Cook Dec 18 '13 at 14:51
add comment

closed as off-topic by Daniel Cook, Malachi, svick, Morwenn, Jeff Vanzella Dec 18 '13 at 17:24

  • This question does not appear to be a code review request within the scope defined in the help center.
If this question can be reworded to fit the rules in the help center, please edit the question.

3 Answers

up vote 8 down vote accepted

You're not using multiple lines of code in one if without curly braces, only one. For example, your foreach loop is considered as one statement, even if the code inside your loop is on multiple lines. That's why you don't need curly braces for your first if.

Same thing for if (row.DisplayElement.ParentGroup != null), which has only one statement : if (row.DisplayElement.ParentGroup.Category != null). It doesn't care about what is inside your second if.

Also, i suggest you to avoid nesting loops and if statements like that because it can become hard to read when your statements get bigger.

share|improve this answer
add comment

The problem with massively nested if statements without the { } is that it is a fragile structure. It is easy to come in and add a new statement and not realize whether it is or is not in one of the ifs or not. It also makes else statements harder to follow as not everyone is completely familiar with the precedence rules for else statements.

It works, it's entirely valid code - but it is bad style.

In this particular case you don't even need the nested ifs:

if (ggcCompareGrid.TopLevelTable != null)
{
        foreach (GridRow row in ggcCompareGrid.TopLevelTable.Rows)
        {
            if (row.DisplayElement.ParentGroup != null && 
                  row.DisplayElement.ParentGroup.Category != null && 
                  (int)row.DisplayElement.ParentGroup.Category == key && 
                   row.Record == null)
            {
                  row.DisplayElement.ParentGroup.IsExpanded = false;
                  break;
            }
        }
  }

You could also consider in this case having a private boolean function that does the comparison:

 private boolean checkParentGroup(ParentGroup group, int key, Record record) {
      return ((group != null) && (group.Category != null) && 
                  ((int)group.Category == key) && 
                   (record == null))
 }

if (ggcCompareGrid.TopLevelTable != null)
{
        foreach (GridRow row in ggcCompareGrid.TopLevelTable.Rows)
        {
            if (checkParentGroup(row.DisplayElement.ParentGroup, key, row.Record)
            {
                  row.DisplayElement.ParentGroup.IsExpanded = false;
                  break;
            }
        }
  }

This makes every part of the code simpler and clearer.

Obviously the checkParentGroup method should have a better name though, describing what it is actually checking.

share|improve this answer
    
you have an Extra "Curly" Brace in both blocks of code. –  Malachi Dec 18 '13 at 15:37
1  
Well spotted, thanks :) –  Tim B Dec 18 '13 at 15:39
add comment

In addition to other comments about the multi-line blocks, I think some of the more obvious issues are being ignored.... Why do you need these nested if blocks at all? You are just daisy-chaining logic that can easily be short-circuited in a single if condition.

You code should/could be easily rewritten as (even if you use multiple-line structures):

 if (ggcCompareGrid.TopLevelTable != null)
        foreach (GridRow row in ggcCompareGrid.TopLevelTable.Rows)
            if (   row.DisplayElement.ParentGroup != null
                && row.DisplayElement.ParentGroup.Category != null
                && (int)row.DisplayElement.ParentGroup.Category == key
                && row.Record == null)
            {
                row.DisplayElement.ParentGroup.IsExpanded = false;
                break;
            }
share|improve this answer
    
you have an extra parenthesis in there, and this is almost the same answer that TimB gave –  Malachi Dec 18 '13 at 15:36
    
I see the difference in the code now. –  Malachi Dec 18 '13 at 15:39
    
+1 for daisy chaining –  Mat's Mug Dec 18 '13 at 15:43
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.