Omitting indentation/brackets in nested for loops - bad practice/styling? [closed]

Question

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?

Answer 1

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.

Answer 2

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

foreach (var charge in carts.Select(c => c.items).Select(i => i.charges))
{
///...
}

Answer 3

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.

Answer 4

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.