6

I know this is a matter of style, hence the subjective tag. I have a small piece of code, with two nested conditions. I could code it in two ways, and I'd like to see how more experienced developers think it should look like.

Style 1:

while (!String.IsNullOrEmpty(msg = reader.readMsg()))
{
    RaiseMessageReceived();
    if (parseMsg)
    {
        ParsedMsg parsedMsg = parser.parseMsg(msg);
        RaiseMessageParsed();
        if (processMsg)
        {
            process(parsedMsg);
            RaiseMessageProcessed();
        }
    }
}

Style 2:

while (!String.IsNullOrEmpty(msg = reader.readMsg()))
{
    RaiseMessageReceived();
    if (!parseMsg) continue;

    ParsedMsg parsedMsg = parser.parseMsg(msg);
    RaiseMessageParsed();
    if (!processMsg) continue;

    process(parsedMsg);
    RaiseMessageProcessed();
}

(Side question: how do I put empty lines in the source code sample?)

Jon Seigel
  • 12,251
  • 8
  • 58
  • 92
Hosam Aly
  • 41,555
  • 36
  • 141
  • 182

22 Answers22

22

I prefer Style 1 - with the indenting.

Oddthinking
  • 24,359
  • 19
  • 83
  • 121
14

I prefer Style 2 - with the continue statement.

Oddthinking
  • 24,359
  • 19
  • 83
  • 121
  • Since the equivalent option for style 1 got 7 votes, I see no reason to downvote this. Upped. – Adriano Varoli Piazza Dec 24 '08 at 14:28
  • After spending a couple of years with a 'custom code editor' integrated into a work application -- that didn't support the *tab key* or a fixed width font, never mind indentation -- seeing several layers of curly brackets gives me nightmares. Of the two options given, I have to pick #2. – Trevel Mar 20 '09 at 06:07
  • All of the indentation of style 1 is very easy to follow at a glance but the same thing results in arrow code which makes my eyes bleed. My aversion to arrow code pushes me to use style 2. – Dinah Apr 04 '10 at 06:34
8

In principle I agree with the majority who prefer style 1. This is what Steve Mcconnell endorses in "Code Complete" - say what you mean, i.e. if you are more interested in the condition being true, while the false state is rarer or not preferred, then state the preferred version.

In practice though I often find myself using style 2, because I like to weed out all the possible error / invalid states first. After I get rid of all the possibilities I am not interested in, I can write the heart-of-the-matter code down to the end of the routine without constantly wondering if I need to guard against some condition or other. Basically, the attitude is, get rid of the chaff, then do the real work in peace.

Marek Jedliński
  • 7,088
  • 11
  • 47
  • 57
  • 1
    i think mcconnell endorses eliminating indentation. – Dustin Getz Dec 24 '08 at 16:04
  • also note that style2 may have the loop contents extracted into a method, and that method uses early return. http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement – Dustin Getz Feb 18 '09 at 14:18
5

Both of these are bogus. Do not put assignment in conditional expressions.

(!String.IsNullOrEmpty(msg = reader.readMsg()))

You only do this because of the querky behavior of the reader - why does the reader give you a non-message to indicate that reading is done? Here's an alternative with a better designed reader:

while (reader.HasMessage())
{
  string msg = reader.GetMessage();
  HandleMessage(msg);
}
Amy B
  • 108,202
  • 21
  • 135
  • 185
  • Well, it looks to me like a way to avoid having to write the assignment twice, before and after the while keyword. What's the better alternative? – Hosam Aly Dec 24 '08 at 15:36
  • +1: Have to agree, assignment inside of a conditional expression reaks of "look how clever I can be" syndrome. – Juliet Dec 24 '08 at 18:38
  • Not necessarily "how clever I can be", as much as it can be "how the library didn't provide an adequate interface". Thanks for the suggestion David! – Hosam Aly Dec 25 '08 at 06:13
4

I definitely prefer the first version. The continue statement is very nice when not overused.

I'd treat this along the same lines as multiple return statements. They are good for guard clauses and have good usefulness when clarity is improved, but shouldn't be overused.

Also, two spaces on a line should insert a line break for you in code blocks.

Michael Haren
  • 105,752
  • 40
  • 168
  • 205
3

I prefer Style2. Moreover same sample is described at Refactoring.com Replace Nested Conditions

Oleg
  • 1,100
  • 2
  • 11
  • 16
  • Voting up because I agree this code needs refactoring, although I'm not sure if Style2 is the right answer. – Jay Bazuzi Dec 24 '08 at 16:54
3

I want to Refactor this code in a different direction. Your code is doing too many things!

  1. Reading input
  2. Iterating
  3. Raising notifications
  4. Parsing (conditionally! (with notification!!))
  5. Processing (conditionally! (with notification!!))

I think we need some separation here. I want to explore:

  • Moving the reading input in to an iterator (yield return).
  • Moving conditions in to the Strategy Pattern
Jay Bazuzi
  • 45,157
  • 15
  • 111
  • 168
  • I will certainly explore this option, merging it with David B's answer and implementing IEnumerator. I shall read about the pattern now, but wouldn't this have a relatively large overhead? (This loop is at the heart of the application, and it needs to run as fast as possible.) – Hosam Aly Dec 25 '08 at 06:26
  • "thou shalt not optimize without profiling data". My guesses about performance are usually wrong; yours probably are, too. Another thought: if you have a code block that requires braces (because it's multi-line), you have an opportunity for refactoring. Be merciless! – Jay Bazuzi Dec 26 '08 at 16:48
2

In the example shown, I'd go with style 1. If my method were big enough that the nesting became a problem (and there wasn't a better way to refactor the code), then I'd consider style 2. But for just the two compact cases shown, definitely style 1.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
2

Style 1 is simply CLEARER, IMHO. Nothing against Continue per se, but as people have said earlier, the indentation makes for easier following.

2

I personally prefer style 2 for a number of reasons;

  • You can hit a potention bug where you indent after the if but forget the braces.

  • You have less risk of hitting a floating else problem, i.e. with multiple nested ifs you can get confused with which if a given else belongs.

  • You avoid excessive indenting that can lead to code going off the page

When I am coding this, I tend to put the continue on a seperate line;

if (!parseMsg) 
    continue;

For the reasons it makes it more visible, and it is easier to assign a break point to it.

SmacL
  • 22,555
  • 12
  • 95
  • 149
2

style 2 lets the human reader focus on the important code, and not even look at anything that isn't relevant - you see the continue statement and anything below is irrelevant.

style 2 manages complexity and thus scales to more complicated methods, style 1 quickly becomes unmanageably complicated.

Dustin Getz
  • 21,282
  • 15
  • 82
  • 131
  • ote that style2 may have the loop contents extracted into a method, and that method uses early return. http://stackoverflow.com/questions/36707/should-a-function-have-only-one-return-statement – Dustin Getz Feb 18 '09 at 14:19
1

I find continue statements make the code harder to follow (and hence debug). There are cases where you might use them anyway, but I don't think your example is one of those cases.

Jon B
  • 51,025
  • 31
  • 133
  • 161
1

Although I prefer style 1. I find it useful to use style 2 sometimes as it is helpful to reduce the level of indentation and make the code a bit easier to read.

Either style is good to be honest, it's really down to personal choice.

Matt
  • 488
  • 5
  • 12
0

I prefer style 1 with the indenting, it is cleaner, and easier to understand just by looking at it I can see the layout of the code.

JoshBerke
  • 66,142
  • 25
  • 126
  • 164
0

The first style is certainly clearer -- it says what you (apparently) mean, ie, get a message, and if the parseMsg flag is set, then try to parse it; if that's successful, process it. On the other hand, the second version is going to process significantly fewer instructions, especially if parseMsg is set. I'd be tempted to go halfway between the two:

while (!String.IsNullOrEmpty(msg = reader.readMsg())){
    RaiseMessageReceived();

    if (!parseMsg) continue ;

    ParsedMsg parsedMsg = parser.parseMsg(msg);
    RaiseMessageParsed();
    if (processMsg){
       process(parsedMsg);
       RaiseMessageProcessed();
    }
}

...on the theory that you're skipping all the parsing in the special case of parsing being turned off.

Charlie Martin
  • 110,348
  • 25
  • 193
  • 263
  • May I ask how the second version would process fewer instructions than the first one? When I wrote similar code in C++ long ago I remember the disassembly was almost the same. Am I missing something here? – Hosam Aly Dec 24 '08 at 15:14
  • Well, I hadn't had my first coffee yet, and we might quibble about "significantly", and it depends somewhat on the optimizer, but it's at least likely to have two branches in place of one. – Charlie Martin Dec 24 '08 at 15:29
  • I wonder why every time I think that C#'s compiler has to do some optimization "because it's common sense" I find the contrary? Everything is left to the JIT! :( Long live C++! Longer live Java! – Hosam Aly Dec 24 '08 at 15:40
0

I prefer style 1 and equate the continue statement to a goto statement. I admit that it might not be as efficient during execution as the other, but I find that code comprehension efficency almost always is more important than code execution efficency.

John Clark
  • 81
  • 2
  • 6
  • "Almost" being the operative word. This *looks* like something that would be the inner loop of a long-running process. Of course, if the messages are, say, coming across the net, the advantage would be submerged in net traffic. – Charlie Martin Dec 24 '08 at 15:31
0

Before worrying about cosmetic irrelevances, I would remove the use of global variables to communicate between functions. Presumably your language supports return values from functions. Use these to either

  • express success or failure of each step, or
  • have each transition function return the next state

whichever is clearer.

fizzer
  • 13,551
  • 9
  • 39
  • 61
  • Thanks. These were actually returned from event handlers, but I removed the functions to clarify the sample. No global variables are actually involved. Thanks for the suggestion anyway. – Hosam Aly Dec 25 '08 at 06:21
0

My preference would be Style 1 but I would use Style 2 in a heartbeat if !parseMsg or !processMsg happened more often than not. Always put the most likely scenario first - right?

Perry Neal
  • 765
  • 4
  • 7
0

Guard Clause works well for a conditional function return because it creates a complete statement. At one glace you know what is going on(we are done here).

The continue statement generally requires a bit more thought. I personally feel if your using more than one continue statement in a loop you're doing something wrong.

The first type in my opinion is self documenting and the standard way. When ever you go against the standard way you need to add comments.

Cadoo
  • 807
  • 5
  • 13
0

I find the style 1 more easy to follow. So I prefer style 1.

Ken Yao
  • 1,506
  • 1
  • 13
  • 24
0

Both choices are awkward. Start with style 2, extract the inside of the while loop into a new method, and change the continue statements to return statements. And it would still need more refactoring.

The continue keyword is awkward, but the return statement is well understood. This option would have almost no nested indentation.

Craig P. Motlin
  • 26,452
  • 17
  • 99
  • 126
-1

Avoid continue statements where possible.

We avoid goto as much as possible, don't we - doesn't it also make sense to avoid it's cousin continue?

matt b
  • 138,234
  • 66
  • 282
  • 345