Stacking keywords on top of each other - poor style? [closed]

Question

Closed. This question is opinion-based. It is not currently accepting answers.

---

Want to improve this question? Because this question may lead to opinionated discussion, debate, and answers, it has been closed. You may edit the question if you feel you can improve it so that it requires answers that include facts and citations or a detailed explanation of the proposed solution. If edited, the question will be reviewed and might be reopened.

Closed 11 years ago.

Improve this 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?

• AakashM: What seems questionable here is making a service call while holding a lock!

Answer 1

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.

• Konrad Morawski: +1 for Monitor. It's less idiomatic but certainly good to have it up your sleeve
• Cole Campbell: The statement that your code "does the same thing" as a lock statement is incorrect. Section 8.12 of the C# language specification describes the code that is generated by the compiler: it calls the Monitor methods in a specific way to avoid potential race conditions.
• kevin cline: -1. lock(x) and using were created to eliminate the ugliness of try...finally. They handle the acquisition and release reliably in a single place. It is all too easy to forget to release a manually acquired resource.
• Dr. Wily's Apprentice: The client.Dispose() call can throw an exception, which would mean that the Monitor.Exit(serviceLock); statement would not execute. I don't think this is equivalent to the original code. (I don't like the style of the original code, though, and would prefer the indentation.)
• Reactgular: Monitor is what the MSDN says to use if you don't want to use using, and the code is their example. I never use Monitor but I offered two options. Break it down into smaller functions or use Monitor. msdn.microsoft.com/en-us/library/de0542zz

Answer 2

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.

• Dr. Wily's Apprentice: I would suggest using caution regarding changing the tab spacing options in your IDE; all team members should use the same settings, otherwise your code will become a mess of different styles (some code indented with 2 spaces, some with 4, some with tabs). Otherwise, +1 for this post. I find alternative styles that attempt to condense code to be harder to read (i.e. takes longer for my brain to parse), since I can't take advantage of the semantic cues that conventional code structure/style provides.

Answer 3

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 4

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.

• Pieter B: I like your first example but not your second.

Answer 5

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 6

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.

• Konrad Morawski: You confused the using directive (the equivalent of Java's imports - and in C# you can't put it anywhere else than the top of the file, either) with the using statement (which is what the OP used). Entirely different beasts. See msdn.microsoft.com/en-us/library/sf0df423.aspx and msdn.microsoft.com/en-us/library/yh598w02.aspx and stackoverflow.com/questions/2943542/using-keyword-in-java
• Ross Patterson: Yeah, he is, but +1 for "The lock is applying until the end of the catch block and needs to look like it.", and for the fact that he probably would have said the same for using if it weren't for C#'s silly dual uses of the keyword.
• djechlin: @KonradMorawski fixed (weeks later)