If/else or just if?

Today in a code review we had a statement like this:

Statement A

if (typeof val === 'string') {
    return 'A'
} else if (val === null || val === undefined) {
    return 'B'
} else {
    return val
}

And a suggestion was made to switch to the following:

Statement B

if (typeof val === 'string') return 'A'
if (val === null || val === undefined) return 'B'
return val

I won't tell you where we came out 😜, but which do you feel is a better way?

Was the suggestion to be concise and avoid if/else logic a good one, or was the original way better?

Comments

[JasperHorn]

I wouldn't do sample B as written because of the lack of braces for the if, which I'd always add (as well as spreading it out over more lines).

With that change, though, it's something that I do wonder about myself from time to time. With the exact sample given (where the else is just another return statement), I would usually go with sample A because the if-else-chain (which has a known mental model for me) nicely describes what happens. However, when the "else" portion is much larger (i.e. it's the "real implementation" after the corner cases have been taken care of) I'd probably go with something like sample B because it limits the levels of indentation that don't add information.

Ultimately, though, I'm not entirely consistent in this.

[Luka Vidaković]

Statement B in general, because of indentation and context, even though you need to be careful about conditional cases. It's generally easier to follow through. IT REQUIRES MUCH LESS COGNITIVE LOAD because you don't need to think about all the cases that might have happened before or might happen afterwards if the specific condition is true or false. It's mostly used when you want to quickly check for a very bad usage of a function. Parameters that might break your logic. In that case you'd have 1-2-3 if clauses that check for bad params and immediately return/throw error. Alternative to that is to let the bad param checking leak to ALL of you NESTED conditional cases and make your code very hard to read. In that case you need to check for valid params inside of nested code and it's quite possible that similar code will occur a couple of times in the same function, which is crazy.
If you at any point end up thinking that it's not enough, that you need to take care of a bunch of complex cases, your function needs to be broken down to a smaller and composable functions that each do 1 thing and then revise all of the surrounding logic.

[Paul]

I do like the if/else case. Where sometimes when you are actually reading the codes it gives a better insight on the app and just like piece of orchestration or a music it makes sense (it rythms). It does sound and it is a good coding practice.

[Renato Byrro]

I find this version even more difficult to read...

[PatricNox]

Is the if and elseif statement about same scenario?

Yes.

Is the if and elseif statements not about same scenario OR is taking up more than 10 lines?

Switch case.

[Likhita]

I either user Ternary operator or the statement A type of syntax. But after reading this thread definitely will stick to statement B or ternary operator depending on the situation.

[Savas Vedova]

In Golang if the if statement is returning, the linter will complain about the else statement. In short,

if statement == true {
  return 
}

// This is the else statement

[Sebastian Vargr]

I tend to use a small map, or a ternary in these situations.

If’s without brackets breaks the bracket consistency for me, and ‘if else’ can be hard to read.

[Kevin Sullivan]

Why is val not already typed? It could be anything?!? I'd rather look into avoiding either of these situations and make sure the type of val is already known.

[The Jared Wilcurt]

There isn't much value in the else since the first conditional results in a return, however having it aids in readability in this case by grouping these two similar conditions. but having a final return outside of a condition is a best practice to ensure you don't mistakenly return undefined. The trailing else should be removed if it is just a return statement.

function (value) {
  if (typeof(value) === 'string') {
    return 'A';
  } else if (value === null || value === undefined) {
    return 'B';
  }
  return value;
}

Though in many cases, a map works better than a bunch of if/else's.

function (state) {
  const states = {
    indiana: 24,
    ohio: 36,
    florida: 19
  };

  return states[state] || 0;
}

Using the mapped approach eliminates any need for switch statements.

Also, don't abbreviate your code, there is no benefit gained from shaving off a few characters. Let the uglifier worry about that. Your goal should always be readability above all else until something is noticeably slow.

[Matei Adriel]

I usually go for the if + else if and no else at the end

[Heithem Moumni]

I prefer the second, It's clean and readable :D

[Ravavyr]

I see a ton of comments on this thread and i read through about half.
I don't see anyone making a case for method A.

I prefer method A because conditionals rarely do just one thing inside them like these comments would have you think.

eg.
if(something){
//one line here
}else if(anotherthing){
//one line here
}

If this were always the case, then having just if statements after each other is simple and easy to read and maintain.

More often than not it's "//50 lines here" in between the conditions, and if you're not using squigglies (brackets) then your code quickly becomes unreadable. You won't know what conditional you're inside of even with indentation.

And yea, someone's gonna go "You should never have 50 lines inside a conditional!"
Ok, so you'd have a conditional that calls a method that calls another 5 extracted methods in other files right? because THAT solves the problem, when it really just makes your code more convoluted and harder to follow.

Also, does it really hurt so much to just write "else" in front of an if statement? So you always KNOW that this is A. not the only condition, and B. not the last condition either.

In the long term it's never good to minimize your code like that. You always end up with someone going in to add something to the conditional and because they added a second line it now breaks because of a lack of squigglies. The code executes the second line always because it's not "inside" the if statement.

It's just better practice to always wrap things so they're maintainable in the long term.

My 2 cents.

[Simon Massey]

interesting that modern languages are putting in generators and "yield" that have multiple exits that save the stack and then reenter into a new position with the saved stack.

[Simon Massey]

I vote for a mixed style: return early if there is an obvious default case (a guard) then use an if/else for some set of related cases. My reasoning is below.

I first ran into this waaaay too long ago in a code review with someone who said "a method should have one exit point" whereas i was liking the "exit early" or "guard" approach. I argued "but if you use early returns you don't have the cognitive overhead of needing to reading the whole method". Yet to them "single return" was The One True Way so we agreed to differ. My guideline for what to do when two devs disagree on a purely stylist matter is that the code remains as-is. I actually cannot remember whether it was my code or their code that was under review.

Unfortunately early returns is often a rotten code smell of "couldn't be bothered to refactor to add a new feature". In that anti-pattern you have a God Object / God Function and to add new features you add a fresh block at the top of the enormous "doAnything" method to detect your new scenario, call your new feature logic in another file, then return early. Then "ta da!" you never have to refactor the God code or change its unit tests (which are incomplete) nor manually test anything other than your new feature case. This leads to an immediate Broken Glass model where the next dev copies that approach. Pretty soon you have a thousand line method that keeps on growing with every new feature added. Regrettably large OO code bases that have seen any team turnover tend to default to this sort of thing remarkably fast.

If you tempted to return early in more than a single obvious default case then perhaps you should break up the class or method into different units that can be composed where each unit has an if/else set for some closely related cases. You then end up with smaller functions or classes that handle a tight set of cases. Then you can take a mixed approach. Try to only return early if there is a really common default case. Why? Because during debugging you don't want someone to have to read the whole code if there is a very common case. The more early returns you put in the more the code is asking to be broken up. The more complex the if/else starts the more you should break it up into different units that handle a more narrow set of cases.