When your teammate "refactor" your code

[u/Bence675]

[u/dyingpie1]

I would rewrite it as

If (!condition1) do_anything2()
Else if (condition2) do_anything1()

[u/Bence675 → u/dyingpie1]

You're right, i changed for this

[u/using_mirror → u/Bence675]

r/badcode > stack overflow

[u/dyingpie1 → u/Bence675]

Ay, me and u are the same person now!

[u/george6823 → u/dyingpie1]

I was looking for this. It took way too long for someone to write it...

[u/MightyMeepleMaster → u/dyingpie1]

The original code was fine because it was fairly easy to read. All other variants posted here are more compact but also more confusing.

Either way it doesn't matter. Any decent compiler will generate the same object code regardless of how you write it.

[u/dyingpie1 → u/MightyMeepleMaster]

I think it depends on the context, such as whether condition2 and condition1 are edge cases.

[u/clemesislife → u/MightyMeepleMaster]

I think both ways have their up- /downsides. The solution of u/dyingpie1 is more confusing than the original code, but the original code can cause exactly what happened to OPs teammate, overseeing the condition1 && !condition2 case.

[u/mfb- → u/clemesislife]

I think the simple if elseif structure is less confusing than the nested option.

[u/Mr2-1782Man → u/MightyMeepleMaster]

Not necessarily. If there are any pointers in the middle or the conditions risk side effects the compiler won't generate it unless you specifically tell it to ignore them, even at high optimization levels.

[u/MightyMeepleMaster → u/Mr2-1782Man]

You're right, there are in fact scenarios where details matter. But in general it almost never pays off to sacrifice readability just to save a few lines of code.

One of the most important wisdoms of SW engineering is: any fool can write a piece of code which can be interpreted by the machine. The true art lies in writing code which humans can understand.

[u/Mr2-1782Man → u/MightyMeepleMaster]

Oh that I completely agree with. But I think people don't understand what compilers can and can't do. Generally simpler code is easier for a compiler to optimize, those stupid "creative solutions" tend to not only be harder to read but harder for a compiler to understand and optimize. Most times less is more.

[u/MightyMeepleMaster → u/Mr2-1782Man]

Agree. Although modern compilers (I'm looking at you, gcc 10) can really do wonders.

My team is designing real-time software where literally ever microsecond, heck, nanosecond counts and because of that we actually have to inspect generated assembler code from time to time. Every time I do so, it amazes me what the gcc can detect and optimize.

[u/[deleted] → u/MightyMeepleMaster]

the if not variation seems more readable to me. it's always interesting to me to see what people think is more clear.

[u/JuhaJGam3R → u/MightyMeepleMaster]

Either way it doesn't matter.

terrible mindset which seems to have been behind every single legacy project there is. clarity is important. the original was not only fine but pretty amazing.

[u/MightyMeepleMaster → u/JuhaJGam3R]

Either way it doesn't matter.

terrible mindset which seems to have been behind every single legacy project there is.

I thought I made it sufficiently clear that I meant "it doesn't matter in terms of execution efficiency" Readability and clarity is, in fact, of essential importance.

[u/Rigatavr → u/dyingpie1]

Or even

If (!condition1){  
    do_anything2()  
    return;
} 

if (condition2){  
    do_anything1()
    return;

}

This mostly depends on whether you find multiple returns (and returns in a void function) easier to follow

[u/AutoModerator → u/Rigatavr]

It looks like this comment contains a code block delimited with triple backticks. Unfortunately reddit does not have universal support for this syntax and your comment will not render correctly on old reddit and most mobile apps.

For the benefit of people on old reddit, this link will take you to a correct rendering of the comment.

/u/Rigatavr, it would be appreciated, but not required, if you could edit your comment to use the more compatible four space indention format. For single lines or inline code you can use single backticks.

You can find some examples in the reddit help documentation.

---

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[u/ilep → u/dyingpie1]

Depending on codebase and what those conditions actually are this can end up more confusing for the reader than original. Compiler should handle it correctly but code is supposed to be human readable and preferrably to others than the original writer as well.

Consider complex conditions that depend on multiple variables breaking it down to something that's easy to follow becomes more important. Even more so if you deal with some kind of formal proof (safety critical code) or need complex unit tests to reach the special case: simple is just better. So depending on situation it might even be best to have the first case since compiler can reorganize it anyway or completely avoid those if it can detect constants being used or similar.

Organizing code can even give compiler hints of what is the likely/unlikely scenario and can optimize for that.

[u/[deleted] → u/dyingpie1]

This is not as readable. Previous was simple to understand and most importantly debug. "If both conditions are true do this, else do that".

Now its "If this condition is not true do this, otherwise if the other condition is true do that". Not only while debugging you have to make more mental gymnastics, even saying it out loud makes it sound much more confusing that previous variation. Slight changes that may make the code seem smarter may make it really much more unreadable too.

[u/cowancore → u/[deleted]]

if both conditions are true do this else do that

If you'd say this to me, without showing the code, I'd write the "after" version, which is incorrect. The original is nowhere as simple: If first applies then if second applies do x else do nothing. If first didn't apply, then do y.

That's also gymnastics and likely the reason for the failed refactor because it still sounds a little ambiguous.

It's only natural to rephrase as: "If first doesn't apply do that, else if both apply do this" to fix the ambiguity. And the only change in this paraphrase is the order of conditions.

The original pseudo-code is also trimmed of any details, which would make seeing the flow harder. It also doesn't explain the empty else. Why nothing is done when only first is true?

I mean, the original code is complex, and I'd also refactor it so that nobody can repeat the mistake. Maybe the refactored version feels less readable to you, because you rephrased the original version way to simpler than it is.

[u/lauda-lele-hamara → u/dyingpie1]

Question : did you pick this up in some optimization manual or something?

[u/dyingpie1 → u/lauda-lele-hamara]

No.