14

Consider the following code sample:

private void AddEnvelope(MailMessage mail)
{
    if (this.CopyEnvelope)
    {
        // Perform a few operations
    }
}

vs

private void AddEnvelope(MailMessage mail)
{
    if (!this.CopyEnvelope) return;
    // Perform a few operations
}

Will the bottom code execute any faster? Why would ReSharper make this recommendation?

Update

Having thought about this question the answer might seem obvious to some. But lots of us developers were never in the habit of nesting zounds of if statements in the first place...

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
JL.
  • 78,954
  • 126
  • 311
  • 459
  • 3
    You've been misled if you think this recommendation is anything to do with improving performance. It is actually a guard clause that helps reduce the number of code paths in your method. This in turn reduces complexity and makes testing and maintenance easier. – Ash Dec 12 '09 at 00:16
  • 1
    Possible duplicate: http://stackoverflow.com/questions/268132/ – Christian C. Salvadó Dec 13 '09 at 05:12

6 Answers6

47

It doesn't matter. Stop agonizing over performance issues that don't exist - use a profiler to identify areas in your code that DO exhibit issues, and fix them. Proactive optimization - before you know that there is a problem - is by definition a waste of time.

Alex Weinstein
  • 9,823
  • 9
  • 42
  • 59
  • 2
    +1. To Alex you listen, JL. Code for readability and maintainability *first*. If you find a slow spot, *then* reach into a bag of tricks for "clever" code. – Randolpho Dec 11 '09 at 22:27
  • 1
    Actually I'm trying to gain a deeper understanding of language fundamentals. Also playing around with reSharper, and when it makes a change or recommendation I want to know the logic behind the change. – JL. Dec 11 '09 at 22:28
  • 1
    I guess maybe you got the impression that I am out micro managing every line of code - hehe. Well I should update the question to - why would reSharper make this recommendation to change the logic, Aequitarum answered that for me. – JL. Dec 11 '09 at 22:34
  • "Premature optimization is the root of all evil" – Michal Franc Nov 23 '12 at 08:30
  • Don't use too broad sentences that are generally false but specifically correct (in specific situations). This famous phrase by Donald Knuth is widely misunderstood and too highly regarded. Of course most people program for non real time thus they go unaffected. But in my lines of jobs I can tell you that you think about optimization UP FRONT or you are going suffer hell later. This refers to choosing carefully containers mostly, and design to elimitate copies, or allocations. Well explained on c2: http://c2.com/cgi/wiki?PrematureOptimization – v.oddou Apr 17 '14 at 07:55
18

Updated Answer:

It's a code maintainability suggestion. Easier to read than nesting the rest of the code in an IF statement. Examples/discussion of this can be seen at the following links:

Original Answer:

It will actually run (very negligibly) slower from having to perform a NOT operation.

So much in fact, some people actually consider that prettier way to code as it avoids an extra level of indentation for the bulk of the code.

Brett Allen
  • 5,297
  • 5
  • 32
  • 62
  • 1
    This was a resharper recommendation. I'm more for performance than pretty code to be honest... – JL. Dec 11 '09 at 22:26
  • 6
    Yup, the difference here is whether it's performing a NOT or not. The advantage, though, is that you have 1 fewer level of nesting, which may increase readability depending on the situation. – Will Eddins Dec 11 '09 at 22:27
  • 13
    @JL: That is such a premature micro-optimization that it boggles my mind. Legible, easier-to-read (thus easier-to-maintain) code should always be the priority unless you have proven performance issue. See Alex's answer (http://stackoverflow.com/questions/1891259/c-will-inverting-ifs-give-better-performance-even-slightly/1891270#1891270) -- it is the correct one. – John Rudy Dec 11 '09 at 22:28
  • 1
    @JL: Don't worry about performance here, it'd come out nearly identical. It's the same performance difference of doing `if (myBool == false)` vs `if (!myBool)` i'd assume. – Will Eddins Dec 11 '09 at 22:29
  • I'll accept this answer because it does answer my question. I guess resharper opts for the readability. – JL. Dec 11 '09 at 22:32
  • That's not necessarily true. While I haven't looked at the CLR, I presume it compiles down to jump instructions of some form, and you can simply jump the opposite way if you're notting the boolean, assuming there's at least some primitive optimizer. But this is a moot point anyways, since that performance change is going to be minute, unless you're running a really tight loop in machine code, which you aren't. – McPherrinM Dec 11 '09 at 22:34
  • I would rather say it was a ReSharper "suggestion" than a "recommendation". ReSharper adds a l of options in your code you can do (but not need to), eg. it suggests using String.Format if you are concatenating strings. Not all of these suggestions will actually improve the code, it is just something you can do with this piece of code. – Olli Dec 11 '09 at 22:40
  • In Release build on C#4, the NOT version is slightly faster. In Debug, the other version wins. This is performance micro-managing though. – Robert Jeppesen Dec 11 '09 at 22:41
  • 5 more upvotes on this guy and Alex gets a Populist badge. :) – John Rudy Dec 11 '09 at 22:47
  • 5
    Why would this have to perform a NOT operation? Assuming an x86 CPU, this should be compiled to something like a compare instruction (cmp) followed by a jump-if-equal instruction or a jump-if-not-equal instruction (je/jne). Two instructions, either way. There might be a small difference due to branch prediction, but that could easily go either way. – Niki Dec 11 '09 at 22:51
  • It makes sense that a jne might be faster, as it doesn't necessarily have to look at the complete value. – Robert Jeppesen Dec 11 '09 at 23:07
  • @Robert Jeppsen: jne doesn't look at the value at all. The compare instruction "looks" at the value and sets some processor flags (equal, less-than, carry). jne/je look just at the equal flag. And code executed after the jump can rely on all flags being set, so there's no shortcut to decide if a value is zero. Anyway, I'd assume an x86 CPU does that comparison in one cycle. – Niki Dec 12 '09 at 09:51
12

It's a refactor of a conditional that encompasses the entire method contents to a Guard Clause. It has nothing to do with optimization.

Forgotten Semicolon
  • 13,909
  • 2
  • 51
  • 61
  • 2
    You mean it has nothing to do with *performance* optimization. It definitely is about optimizing ease of maintenance and readability. – Ash Dec 12 '09 at 00:11
3

I like the comments about optimizing things like this, to add a little more to it...

The only time I can think of that it makes sense to optimize your if statements is when you have the results of TWO or more longish running methods that need to be combined to determine to do something else. You would only want to execute the second operation if the first operation yielded results that would pass the condition. Putting the one that is most likely to return false first will generally be a smarter choice. This is because if it is false, the second one will not be evaluated at all. Again, only worth worrying about if the operations are significant and you can predict which is more likely to pass or fail. Invert this for OR... if true, it will only evaluate the first, and so optimize that way. i.e. 

if (ThisOneUsuallyPasses() && ThisOneUsuallyFails())

isn't so good as 

if (ThisOneUsuallyFails() && ThisOneUsuallyPasses())

because it's only on the odd case that the first one actually works that you have to look at the second. There's some other flavors of this you can derive, but I think you should get the point.

Better to worry about how you use strings, collections, index your database, and allocate objects than spend a lot of time worrying about single condition if statements if you are worrying about perf.

In general, what the bottom code you give will do is give you an opportunity to avoid a huge block of code inside an if statement which can lead to silly typo driven errors. Old school thinking was that you should only have one point that you return from a method to avoid a different breed of coder error. Current thinking (at least by some of the tool vendors such as jetbrains resharper, etc) seems to be that wrapping the least amount of code inside of conditional statements is better. Anything more than that would be subjective so I'll leave it at that.

Jim L
  • 2,297
  • 17
  • 20
  • Are you sure an OR will always evaluate both conditions in C#? Since an OR is satisfied if either of the two conditions is TRUE, the OR evaluation should short-circuit if the first condition is TRUE, no? An XOR must evaluate both sides, but that's a different animal.... – Val Dec 12 '09 at 00:59
  • @Val ack... edited. I was thinking in terms of negations and I think ended up in the XOR situation. I blame a case of "Fridays"... – Jim L Dec 13 '09 at 05:14
0

This kind of "Optimizations" are not worth the time spent on refactoring your code, because all modern compilers does already enough small optimizations that they make this kind of tips trivial. As mentioned above Performance Optimization is done through profilers to calculate how your system is performing and the potential bottlenecks before applying the performance fix, and then after the performance fix to see if your fix is any good.

bashmohandes
  • 2,356
  • 1
  • 16
  • 23
  • Interesting point of view... tell that to the makers of ReSharper and the millions of developers out there, who take the time to ReSharp their code :) – JL. Dec 11 '09 at 22:51
  • I wish Resharper came with a big sticker saying, "No matter how smart Resharper is, *it is still dumber than you are.*" So many people seem to do what Resharper tells them "because Resharper told me to do it" without thinking about whether it's actually a good suggestion. – itowlson Dec 11 '09 at 23:14
  • @itowlson, exactly the reason for the question. – JL. Dec 11 '09 at 23:32
  • I would still do some of these refactoring tips for the sake of prettier code, but not for the sake of performance optimization. – bashmohandes Dec 12 '09 at 23:40
-1

Required reading: Cyclomatic_complexity

Cyclomatic Complexity is a quantitative measure of the number of linearly independent paths through a program's source code

Which means, every time you branch using and if statement you increase the Cyclomatic Complexity by 1.

To test each linearly independent path through the program; in this case, the number of test cases will equal the cyclomatic complexity of the program.

Which means, if you want to test your code completely, for each if statement you would have to introduce a new test case.

So, by introducing more if statements the complexity of your code increases, as does the number of test cases required to test it.

By removing if statements, your code complexity decreases as does the number of test cases required to test.

shenku
  • 11,969
  • 12
  • 64
  • 118