-1

Does it decrease performance or is it bad practice if I create scopes which aren't necessary? By that I mean, creating scopes just for readability:

XmlElement header = list[0] as XmlElement;
if (header == null) throw new Exception("Corrupt header.");

{
    XmlElement subHeader = null; // ...
}

This way it is cleary better readable. Any reasons why to avoid this? I just realized this would be quite nice to make my relatively long code a bit more readable. Especially because this happens more than once that I have to load a main element which then has some sub elements I could easily separate visually.

What do the Pros say?

SharpShade
  • 1,761
  • 2
  • 32
  • 45
  • 1
    No, there is no performance impact. You might want to consider breaking up your code into methods though. – Blorgbeard Sep 29 '14 at 23:02
  • 8
    But if you're really after readability, consider *not* including the `if` action (`throw new Exception...`) on the same line as the condition. Especially in this case, where the following block appears at first glance to be attached to the `if` – Paul Roub Sep 29 '14 at 23:04
  • 2
    Scanning the code quickly, I'd read your code block as belonging to the if statement. It's an unconventional format. So for me it's certainly not "clearly better". Hopefully you don't think shorter always means better. – itsme86 Sep 29 '14 at 23:05
  • 1
    If you need to do the same validation for the same scenario many times over, think of rather extending the `XmlElement` class with a custom construstor which takes in the value of `list[0]` as a parameter and does the validation there. Shorter and cleaner code and a single point of validation changes if needed. – Bernd Linde Sep 29 '14 at 23:07
  • 5
    The only scopes that have an impact at runtime are the ones that actually change the logic of your code. Like try/catch, lock {}, using {}. if {}/else {}. Not this kind. Do beware that it is *not* more readable, whomever sees this will lose a minute of his life trying to figure out why you did this. You'll owe him for that minute, very hard to pay back. – Hans Passant Sep 29 '14 at 23:09
  • I know that was a badly chosen scenario. But for exactly that reason I've added the blank line between ;) Bernd Linde, that's a really good idea. Haven't thought about that yet. All in all this was a more or less general question. Splitting up into methods would be a solution, but I personally don't like splitting up unnecessarily. It makes stack traces more complex and you lose overview about your code. Of course this applies only to a scenario like in my sample code. – SharpShade Sep 29 '14 at 23:13

1 Answers1

2

This isn't more readable. Your example code shows this.

In my mind the difference between these two sets of code is confusing:

XmlElement header = list[0] as XmlElement;
if (header == null)
{
    XmlElement subHeader = null; // ...
}


XmlElement header = list[0] as XmlElement;
if (header == null) throw new Exception("Corrupt header.");
{
    XmlElement subHeader = null; // ...
}

Also, if your code is "relatively long" then it should be broken down into separate methods and not grouped by scope blocks.

Now, as far as performance goes. Here's a simple example:

var text = "Hello";
Console.WriteLine(text);

This turns into this IL:

IL_0001:  ldstr       "Hello"
IL_0006:  stloc.0     // text
IL_0007:  ldloc.0     // text
IL_0008:  call        System.Console.WriteLine

If I write the code like this:

var text = "Hello";
{
    Console.WriteLine(text);
}

The IL becomes:

IL_0001:  ldstr       "Hello"
IL_0006:  stloc.0     // text
IL_0007:  nop         
IL_0008:  ldloc.0     // text
IL_0009:  call        System.Console.WriteLine
IL_000E:  nop

Note the nop operations.

For every scope block I get a new pair of nop operations in the IL. But this only happens in debug mode. In release mode the nop operations are removed.

To test the performance difference in debug mode I wrote this code:

var sw = Stopwatch.StartNew();
var x = 0L;
for (var y = 0; y < 1000000000L; y++)
{
    {
        x += y;
    }
}
sw.Stop();
Console.WriteLine(x);
Console.WriteLine(sw.ElapsedMilliseconds);

With the extra scope it ran consistently in about 3,550 ms. Without the extra scope is was about 3,500 ms. So about 1.5% difference in performance. And this is only in debug mode!

But considering that my operation x += y is so trivial and the performance drop so miniscule that you could probably just ignore the performance difference in normal code. And obviously ignore it entirely in release mode code.

Enigmativity
  • 113,464
  • 11
  • 89
  • 172
  • Good to know, thanks for the test. I know it's not much more readable, especially in this scenario (which was badly chosen) but I was curious about it. – SharpShade Sep 29 '14 at 23:35
  • Are you sure you compiled in release mode? I strongly suspect these `nop` instructions are there just to let you set a breakpoint on them (when you set a breakpoint on the additional opening brace for instance). – Lucas Trzesniewski Sep 29 '14 at 23:41
  • @LucasTrzesniewski - Yes. You are right. They go away in release mode. – Enigmativity Sep 29 '14 at 23:45
  • 1
    +1. Well, this changes the conclusion of your post I guess => there's no difference whatsoever. IMO you should even remove the benchmark as it's irrelevant in debug mode. – Lucas Trzesniewski Sep 29 '14 at 23:54
  • @LucasTrzesniewski - Yes it does change the conclusion, but I think that the performance code still highlights the fact that the scope blocks do between nothing and virtually nothing to performance in any case. – Enigmativity Sep 30 '14 at 00:05
  • I mean if the `nop` instructions disappear there's no point in the benchmark because both code samples will compile to the same MSIL in release mode. No need to test that against itself ;) – Lucas Trzesniewski Sep 30 '14 at 00:07
  • @LucasTrzesniewski - Oh yes, I agree. In release code there is no difference of course because it's the same IL, but it's still worth considering the debug code as I'm sure there are plenty of people running debug code in real world apps. – Enigmativity Sep 30 '14 at 03:51