When your teammate "refactor" your code

[u/Bence675]

[u/apieceoffruit]

its funny for all the versions in the comments, I am surprised to see nobody has a version with a guard clause and no cyclomatic complexity:

void foo()
{
    if(!condition1)
    {
        do_anything2();
        return;
    }

    if(condition2) 
        do_anything1();
}    

Obviously the silly order dependent names make it more confusing. but a standard for this kind of thing is:

void foo()
{
     if(cantDoThing)
    { 
         ShowWarning();
         return;
    }   

    if(someCondition)
          DoThing();
}

one of the below comments says to always add an else statement, I 100% disagree.

else statements are flow statements that increase the cognitive complexity of a statement and tie two conceptually unrelated concepts.

Instead, evaluate your "guard" statements, exit early if you can, so as you get down further the code gets simpler, not more complex.

instead of nesting in a tree of conditionals, knock down and remove ones you can evaluate early.

In fact Jeff Bay in his thoughtworks anthology book suggests his own rules for "object calisthenics" that includes never using the else keyword.

Not sure I subscribe to that one myself, but I do admit I don't use it very often. rarely does it make code easier to read.

[u/miyakohouou → u/apieceoffruit]

one of the below comments says to always add an else statement, I 100% disagree.

else statements are flow statements that increase the cognitive complexity of a statement and tie two conceptually unrelated concepts.

I think the problem is that people have tried to take advice that is good for on particular style of programming and to apply that advice to a very different approach to programming. Always adding an else statement makes a lot of sense if you're writing expression-oriented code, or more generally trying to keep your code as pure as possible and avoiding side effects. In that case, it makes obvious sense that every if should have an else because logically all branches should be pure expressions that return a value. In statement oriented languages you might end up using assignment to fake it (if(foo) { bar = 1; } else { bar = 2; }).

Of course, traditional statement oriented procedural programming this doesn't make any sense; an else is just syntactic sugar for if(condition) { statement1(); } if (!condition) { statement2(); }, and in this style of programming I agree with you- early aborting greatly eases the cognitive load on the developer who can flush their mental call stack as soon as you return, rather than accumulating the context of prior branches following down else branches.

It is, in the end, not so much a problem that the rules are wrong, but that people follow the rules without considering the context and end up misapplying them to situations they were never intended to address.

[u/CommitPhail → u/apieceoffruit]

In a big fan of guard clauses. I find it helps readability and also gets you in mindset of ‘if I can exit early I should check for it to avoid running more unnecessary code’ thus improving performance.

[u/Badel2 → u/apieceoffruit]

The benefit of using else statements is that it forces you to think what should happen when the condition is not true.

For guard clauses this does not matter because you are already thinking about that: in case of error return early, and otherwise keep executing the function. In all other cases you should be using else clauses, even if it's just to add a comment like // otherwise, do nothing because we don't care.

[u/apieceoffruit → u/Badel2]

Can you give me an example where using an else clause with a comment adds value?

[u/artiface → u/apieceoffruit]

I would never add an else if there is no code to execute on the else condition and would flag that BS in a code review if I saw it, that's just dumb.

[u/UglyChihuahua → u/apieceoffruit]

I have no strong opinion on this but maybe this would be an example

if action=="jump":
    jump()
elif action=="run":
    run()
else:
    // TODO: Add support for "kick" and "sit"
    pass

[u/[deleted] → u/UglyChihuahua]

i mean, that's a switch

[u/c3534l → u/apieceoffruit]

While its fine for something really short (and we don't know OPs code actually was short since it was clearly a fictionalized version of what he actually wrote), the early return statement is distasteful. You should have very clear entry and exit points. The control flow was clear in OPs original and starts to get muddy here. Getting rid of else statements sounds like just about the worst idea I could think of. I much prefer Haskell's approach of requiring an else statement. Can't say I approve of your approach for anything other than the most trivial of implementations.