Stacking keywords on top of each other - poor style?

Question

I have always wondered about this, especially in C/Java style languages. For example, consider the first 3 lines of this C# code:

lock (serviceLock)
using (var client = new ServiceClient())
try
{
    // Do something
    client.DoSomething();
}

catch (System.ServiceModel.FaultException ex)
{
    // Handle error
    addLog(ex.ToString());
}

This usage has always seemed somewhat questionable to me. However, the alternative is not very practical either (3+ levels deep of indentation for a service call). Apart from pure aesthetics, is there any practical reason to add or not to add the extra brackets in this case?

Answer 1

I would strongly suggested you use three levels of nested indentation. When I look at this code quickly, I only see the catch(){} block, see a match try{} block, and ignore the fact that there is a lock and using statement. Now, when I look at the code more carefully, I noticed that there are keywords hanging out above the try, but their scope may not be immediately apparent to me--are they just for the try block, or are they for the try/catch statement? If you need to add code at some point, I can almost guarantee someone will miss the lock or using statements (the same reason why most coding standards I've seen prohibit placing only one line after an if statement and not using curly braces). I would go with the most specific code, which is three levels of indentation using proper brackets so that the scope is immediately understandable.

If the indentation bothers you that much, I suggest using 2 spaces instead of tabs (which should be an option in your IDE). It's something I picked up at my job, and have really fallen for as I use it. It's much more concise than having a two inch gap because you indented four times.

Answer 2

I'd say I'm against it. I'll come around and add some code after try...catch without noticing that I should have extended the scope of lock.

And, more importantly, this version doesn't really reduce indentation!

It only hides it. Deep nesting is primarily a logical problem, not an aesthetical one.

The fact that we see it as ugly is a result of it being a bad programming practice; not the other way round.

(Aesthetics by itself is relative - eg. this highly upvoted answer claims that "deeply nested structural statements – "arrowheads" – were, in languages like Pascal, once seen as beautiful code.")

Three levels is still fine, but if you reach - say - seven levels, does removing braces from lock and using, and de-indenting, address the issue really?

Quite the contrary, it desensitizes the developer to the problem, thus reducing the chance someone will go ahead and refactor the thing.

Answer 3

For a dissenting opinion, I rather like this practice with one significant change.

lock (serviceLock) using (var client = new ServiceClient()) try
{
    // Do something
    client.DoSomething();
}
catch (System.ServiceModel.FaultException ex)
{
    // Handle error
    addLog(ex.ToString());
}

By not stacking, it forces the eye to read all the way across as one long statement. If the line is too long or does not read well, then you know you want to reorganize the logic.

I often do this with double for-loops and for-loops with try catch blocks.

for (int i = 0; i < x.Length; ++i) for (int j = 0; j < x[i].Length; ++j) try
{
    // each cell in a ragged matrix: x[i][j]
}
catch (/* … */)
{
    // exception in one cell allowing other cells to be processed.
}

Above all else, clarity is the most important thing. If you are rigid with any style, you will end up with ugly and unreadable code in some cases. Be flexible in a way that allows you to highlight the logic. If you don't know if something is clear, then have your co-developers to look at it. Ask them what the code does without explaining it to them.

Answer 4

I have no problem with stacking the locks and usings, but the try-catch should be wrapped in braces. That will require you to think what the scope of the using statement is in this context.

lock (serviceLock)
using (var client = new ServiceClient())
{
  try
  {
      client.DoSomething();
  }    
  catch (System.ServiceModel.FaultException ex)
  {
      addLog(ex.ToString());
  }
}

I stack keywords all the time (mostly using bit also stuff like fixed and unsafe) because sometimes I would be adding six or more braces. There's no way you can read that at a glance and tell which brace matches which. Also it increases the risk that the code for your method won't fit on the screen. That's not good for readability either.

Answer 5

The rule is simple. Indentation needs to be consistent with scope. The lock is applying until the end of the catch block and needs to look like it. I'm not familiar with the using keyword in C# but from your code the same rule looks to apply.

Answer 6

The lock and using blocks are for programmer convenience, but are not required. If you find less indented code easier to read. You can write the same thing using the non-code-block version of the same C# features.

Monitor.Enter(serviceLock);
var client = new ServiceClient();
try
{
    client.DoSomething();
}
catch(FaultException ex)
{
    addLog(ex.ToString());
}
finally
{
    Monitor.Exit(serviceLock);
    client.Dispose();
}

The above does the same thing, but there is no readability issue or confusion as to what it's doing.

The alternative approach is to use blocks and break everything into separate functions.

public function foo()
{
    lock(serviceLock)
    {
        fooService();
    }
}

private function fooService()
{
    using(ServiceClient client = new ServiceClient())
    {
        fooClientTry(client);
    }
}

private function fooClientTry(ServiceClient client)
{
    try
    {
        fooClient(client);
    }
    catch(System.ServiceModel.FaultException ex)
    {
        addLog(ex.ToString());
    }
}

private function fooClient(ServiceClient client)
{
    // work....
}

I prefer the above approach. It looks like a lot more work/code but it's easier to maintain because each function performs a single task. When you get into multiple nested blocks this approach keeps everything only a few indentations.