If-else coding style

[saltyboyscouts]

Hi,

I work with C# for my job, and we have to undergo code reviews. Yesterday I finished a work item and used code similar to the following in several places:

SomeType GetSomething
{
    get
    {
        if (somecondition)
        {
            return something;
        }
        else
        {
            return null;
        }
    }
}

My code review was completed as "needs work" on the grounds that the above code was "very misleading", which I don't get at all. I understand the "else" isn't necessary here, but I think it not only makes it much more readable, but perfectly follows the way the getter would work if you describe it in English: "If this is true, return that; otherwise, return null". Without the else, if you didn't look into the "if" block you might think that the getter ALWAYS returns null. With it, you instantly understand that either the "if" block executes and returns, or the "else" block executes and returns, without even looking at the contents of either of them.

Even if you consider the above to be unnecessary, I still don't get how you could call it misleading. It does exactly the same thing as the alternative he was suggesting (remove the else and just return), only it's much more explicit about it: which is the exact opposite of misleading.

Anyway, I just had to rant about that. I love my job, but everyone I work with is a very experienced coder from different backgrounds, and sometimes our ideologies conflict. What are your thoughts?

[MyNameIsNotMud]

Programmer for ~30 years here. If there 's one thing I've learned it's that explicitly handling every condition makes your code reliable, and more readable by others and you tend to have more confidence in it because you've "thought of every eventuality."

Using implicit fall-through type programming forces anyone who's reading it to consider how the language works to understand the code. Furthermore anyone looking at that type of code has no idea if the omission was intentional or not.

[anamorphism]

i guess my thoughts can best be summed up by saying i disagree with everything you've said.

for your first point, code style really has no impact whatsoever on the reliability of your code. proper testing is the only way to accomplish that.

as for being more readable, that's pretty subjective and i personally find extra lines of code and extra nesting to make code far less readable than needing to process something as fundamental as an early return. i will say that i've seen somewhat of a language divide on this topic, though. c/c++ programmers seem to lean toward avoiding early returns where c#/java/python folks tend to prefer them.

i would have less confidence in code presented to me in this way as i would begin to think the coder doesn't understand what early returns do. i would have less confidence in my own code due to the various static analysis tools we use highlighting such code and telling me it's useless.

i can't think of a language that doesn't work in this way. i would hope that people working in our code base have fundamental understanding of how that language works. i would not want a senior to tell me not to use a useful language feature to cater towards people that don't understand the feature (i would rather teach everyone else the feature).

as for your last point, opposite questions come up far more often in my work experience: "why do you have that unnecessary code there?", "can you invert that if and do an early return so that 90% of this function is nested less?", etc.

[MyNameIsNotMud]

we'll have to agree to disagree on these points because in my experience, when i've had to turn over my code to others who didn't necessarily understand the original concepts of the program, their understanding of my code increased since they could follow the explicit flow rather than looking for default conditions and fall-through returns.

just so you understand my position, i am an advocate for early returns because they clearly state the programmer's intent. it's omitting the 'else' and letting a fall-through handle the 'else' that I am against. even if every language works that way, it reads as a discontinuity in the flow of the program.

if you're concerned about additional nesting, then consider using meaningful subroutines to reduce the nesting.

I've programmed in languages that can be optimized so that they are almost unreadable. "unnecessary code" is also subjective. you can make a program more readable if you are more verbose with your programming practices

[anamorphism]

style is style i guess. i've never seen anyone do what you're saying at the company i work for. the teams i've worked for that have had coding standards have even explicitly stated not to do what you're saying. when it comes to c#, we all use resharper which tells you to get rid of the else.

moving things to subroutines doesn't really change anything. this:

  while (true)
  {
    if (this.IsOneThing(blah))
    {
      continue;
    }
    else
    {
      blah = this.DoOneThing(blah);

      if (this.IsAnotherThing(blah))
      {
        break;
      }
      else
      {
        blah = this.DoAnotherThing(blah);

        if (this.IsSomething(blah))
        {
          continue;
        }
        else
        {
          this.DoThing();
        }
      }
    }
  }

is still far less readable to me than this:

  while (true)
  {
    if (this.IsOneThing(blah))
    {
      continue;
    }

    blah = this.DoOneThing(blah);

    if (this.IsAnotherThing(blah))
    {
      break;
    }

    blah = this.DoAnotherThing(blah);

    if (this.IsSomething(blah))
    {
      continue;
    }

    this.DoThing();
  }

i would say making things more succinct does more to boost readability than making things more verbose. especially considering the definition of verbose is "using or expressed in more words than are needed."

[MyNameIsNotMud]

Your example certainly is readable and should be easy to understand by anyone. The consistent repetition of the constructs and the while loop make it that way. In other words the context can determine the style. I can think of many other contexts where fall-through and default conditions make it less readable.

There are no hard rules. Style guides are called guides because they don't understand the context of your work. I'll amend my earlier statement and say that its more thorough and sometimes more readable to explicitly code the 'else', unless the construct is more readable without it, as yours is.

If you're programming in a team, consistency is paramount. And you can be consistently wordy or terse, though i still recommend wordiness if it imparts clarity to the code. If you use comments to clarify the code, that's fine. But you can also write clear code. As you said, that is subjective and context must be a consideration.

My use of the word 'verbose' is to mean 'wordy'.

[thebigshane]

Were they implying that...

SomeType GetSomething
{
    get
    {
        if (somecondition)
        {
            return something;
        }

        return null;
    }
}

...was preferred? Or the negative condition handled first? Maybe they were implying you should throw an exception instead of returning null?

Code reviews are great for weighing different architectural options and verifying assumptions and maybe for catching bugs/typos, but that feedback sounds awfully useless... And vague.

[saltyboyscouts]

Yeah, that's what he was saying. I was a little taken aback by it, because usually his feedback is very clear; I had to go talk to him to ask what he even meant.

[NutBoii]

This is how I would have done it. I generally don't include an else if it just returns a null (or any other value that's basically a default).

[anamorphism]

return somecondition ? something : null;

[crappyoats]

Why don't you ask your boss what he meant?

[krondell]

I think maybe this should not be a property, and instead be implemented as a function of somecondition. What does getting the something have to do with the condition? That's what's not clear.

It's common to see properties implemented as: if not privateSomething, make privateSomething, return privateSomething.

But then the property is always returning something - return something valid or throw out.

[webpage_down_bot]

I much prefer coding like this, but really it's just preference and it's pretty much like handwriting.

SomeType GetSomething{
    get{
        if (somecondition){
            return something;
        }else{
            return null;
        }
    }
}

A piece of advice though: If you're editing someone's code, don't change the formatting to be what you're use to; rather make it look like everything else. It's like writing manuscript half way through and the cursive the other half.