1

Using C# it's common practice (I think) to omit indentation and brackets with nested using statements like this:

using (var fileStream = new FileStream("filename"))
using (var streamReader = new StreamReader(fileStream))
using (var myCustomReader = new MyCustomReader(streamReader ))
{
    ///...
}

I've been refactorying some code recently that's suffering from messy nesting of if and for loops, I've been using continues/returns to reduce this nesting but I also experimented with using this style with the for loops, so:

foreach (var cart in carts)
{
    foreach (var item in cart.items)
    {
         foreach (var charge in item.charges)
         {
             ///...
         }
    }
}

Becomes:

foreach (var cart in carts)
foreach (var item in cart.items)
foreach (var charge in item.charges)
{
    ///...
}

Which seems much easier to read to me.

Is there a good reason not to use such styling? Is it particularly uncommon/might cause problems with other developers after me?

closed as primarily opinion-based by Pieter B, Bart van Ingen Schenau, Blrfl, gnat, 17 of 26 Dec 21 '17 at 13:19

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • I've never come across anyone doing that; but in theory, I see no reason why it shouldn't be used. Only in theory though: in practice, I'd want to refactor that code to remove the nested foreach's. – David Arno Dec 21 '17 at 11:59
  • 1
    This is mostly opinion based, to be honest; but if I had to choose I wouldn't like either option and I'd go with the linq option or even separate every foreach in different methods. As a rule of thumb I consider excessive indentation a problem, and three levels of indentation just before you do anything else is raising a red flag for me. – Zalomon Dec 21 '17 at 12:01
  • 3
    @Zalomon I agree three levels of indentation are a red flag, and I feel that adds even more to the argument that all foreach-loops should be indented, to provide that code smell as soon as possible! :) – Erdrik Ironrose Dec 21 '17 at 12:54
  • Yeah, even not liking it the three levels are indentation seem to be the best option of the too. Gotta agree with you. – Zalomon Dec 21 '17 at 12:58
  • In case you've never heard of it, Apple had a serious SSL bug introduced into it's OS a few years back because of a missing set of braces (embeddedgurus.com/barr-code/2014/03/…). Other real world examples can be found here: forums.developer.apple.com/thread/5945 – Phil N DeBlanc Dec 21 '17 at 16:22
8

I see a number of issues with only using one set of braces:


You are immediately removing the first mental picture of the loop due to the lack of indentation.

foreach (var cart in carts) {
    foreach (var item in cart.items) {
        foreach (var charge in item.charges) {
        }
    }
}

That tells me "I am dealing with a 3D iteration" and I don't even need to read inside the foreach statements to know that.

Also indentation tells me immediately that I am nested. If statements can often sit next to each other:

if (condition1) {

}
if (condition2) {
   if (condition3) {

   }
}

The nesting is vital in telling me how they relate to each other. In a similar way, nesting is vital to tell me how the loops are interacting with each other as well.


What if there is something that you only want to do in each loop? Do you add the braces for all three or just the two that need it? It's a break in consistency.

foreach (var cart in carts) {
    LogCartStatistics(cart);
    foreach (var item in cart.items) {
        LogItemPurchased(item);
        foreach (var charge in item.charges) {
            // Do the stuff
        }
    }
}

If a line is added mistakenly in the middle like so:

foreach (var cart in carts)
foreach (var item in cart.items)
LogDebug(item);
foreach (var charge in item.charges)
{}

It will break the entire workflow of the loops and it won't compile. You're just giving work to the next person who comes along, who will more than likely add in the braces and indent the loops, which could have been done the first time.


Finally - and most importantly - other programmers will most likely be unfamiliar with your style. They may even consider it to be a mistake (as I would). At the very least, you've broken someone's train of thought as they try to identify what you have done, what you should have done and what you meant to do.

  • I think this is the most sensible set of reasons not to do it. Also answers the question, rather than says just "just use LINQ" or similar (which is still useful advice, but doesn't really answer) – Joe Dec 21 '17 at 13:10
  • @Joe Thanks! It's usually a good practice to wait a day before accepting an answer, just to give everyone a fair chance. :) – Erdrik Ironrose Dec 21 '17 at 13:12
  • I'll sit on it a while longer, but I do think the consensus is a pretty clear "don't do it"! – Joe Dec 21 '17 at 13:14
  • Well, not much point waiting now it's closed! Thanks for the answer. – Joe Dec 21 '17 at 13:30
3

Please don't do that kind of nesting use linq.

foreach (var charge in carts.Select(c => c.items).Select(i => i.charges))
{
///...
}
  • Yeah, I've conisdered replacing it with LINQ, it becomes messy when you need to refer to, for example, the cart or item of the 'higher' loop during the body though. In your example you can only access charge. – Joe Dec 21 '17 at 12:51
  • @Joe It is the same with foreach without braces, if you have something to do there then you need to add braces again. – Mateusz Dec 21 '17 at 12:55
  • @Mateusz The question is why – Caterpillar Dec 21 '17 at 12:57
  • I wouldn't do this because it's easy to overlook as a single loop. – Neil Dec 21 '17 at 13:08
  • @Mateusz not at all, in C# at least. Braces don't affect the scope. – Joe Dec 21 '17 at 13:09
0

I don't see any good reason to use this pattern, and it is not commonly used, so even if you think it is more readable, it will not be familiar to other people reading the code.

The indentation in the middle example makes it clear that we are really iterating in three levels, which is important for understanding the code. I would advice against omitting the braces for the same reason I would advice against omitting braces after if, while etc.

Depending on what you are actually doing inside the inner loop, a Linq expression might be more idiomatic though. foreach is appropriate if the block have side effects, otherwise a Select is more appropriate. In your example, the two outer loops does not have any side effects (since they don't have any code beside the inner loop), so Select is more appropriate for the two outer levels.

I've been refactorying some code recently that's suffering from messy nesting of if and for loops, I've been using continues/returns to reduce this nesting

I would urge you to reconsider your approach. It seems you are not actually reducing complexity, but rather hiding the complexity using syntax tricks.

  • Is it really hiding complexity having a for loop check for an condition if (object == null) continue; at the beginning, compared to have a giant if (object != null) statement (which ends 300 lines later)? I just figured that was good practice. It makes things much cleaner to read (less indentation) and the if statement is short and succinct rather than spanning hundreds of lines. – Joe Dec 21 '17 at 12:46
  • @Joe: If the if block is 300 lines, this is the actual problem. A "continue" is easily overlooked in such a mess, so that just makes it worse. – JacquesB Dec 21 '17 at 13:01
  • Well yeah, hence another reason why I'm refactoring. But I still dissagree about the continue. – Joe Dec 21 '17 at 13:05
-2

The most obvious issue:

if I turn:

foreach (var cart in carts)
foreach (var item in cart.items)
foreach (var charge in item.charges)
{
   ///...
}

into:

foreach (var cart in carts)
var foo=true
foreach (var item in cart.items)
foreach (var charge in item.charges)
{
    ///...
}

now foo is set to true many times, but only the one cart is processed, if any are processed at all.

  • 3
    That is invalid syntax, so won't compile. – David Arno Dec 21 '17 at 12:04
  • 1
    You'd be a lunatic to attempt to insert "var foo=true" there. While I don't like the style, I don't think there is any danger of someone inserting bugs into the code. – Neil Dec 21 '17 at 13:10
  • This is not dependent on the language. Think pseudocode. Every language that allows optional "{}" opens itself up to a bug by somebody inserting an extra line of code. Sometimes it is hard to find. – mhoran_psprep Dec 21 '17 at 13:29
  • 2
    If you accidentally put a ; after one of the foreachs that would compile, no? This has caused some famous bugs in C. – Alex Dec 21 '17 at 13:42
  • 1
    @Alex It does not, because the next nested loop would be out of scope and you get the error cart does not exist in the current context. – Joe Dec 21 '17 at 15:05

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