22
\$\begingroup\$

When coding I noticed I started to get into a habit of not using if-else statements when the else block only has one line of code. For example, if I have code that can be solved like this:

public Person(int initialAge) {
    if(initialAge < 0){
        System.out.println("Age is not valid, setting age to 0.");
        age = 0;
    }
    else{
        age = initialAge;
    }
   }

I will instead remove the else statement entirely to cut down on a few lines of code. This results in my code looking more like this:

public Person(int initialAge) {
    age = initialAge;
    if(initialAge < 0){
        System.out.println("Age is not valid, setting age to 0.");
        age = 0;
    }
   }

I'm wondering if this is a bad coding habit to get into and if I should break out of this coding habit or if it'll be fine to continue doing this. Would doing this be a bigger issue down the line when I get onto more complex programs?

Edit: Just wanted to give some more information for why part of the code is written the way it is. The snippet provided is part of a coding challenge where one of the requirements is to set any negative number inputted to 0.

\$\endgroup\$

migrated from stackoverflow.com Dec 18 '16 at 17:23

This question came from our site for professional and enthusiast programmers.

  • 5
    \$\begingroup\$ Yes, it is a bad habit because it makes your code harder to understand (both for others and yourself in the future when you look back at your code). if-else is very straight forward; this however, is not. \$\endgroup\$ – Gendarme Dec 18 '16 at 17:05
  • 5
    \$\begingroup\$ In your particular case the if-else set up is better. However it almost seems that you should be throwing an exception rather than setting it to 0 \$\endgroup\$ – Ahmed Masud Dec 18 '16 at 17:07
  • 2
    \$\begingroup\$ Whats making this hard to understand is that you are using initialAge to set age before checking its value. \$\endgroup\$ – Jonas Köritz Dec 18 '16 at 17:08
  • 2
    \$\begingroup\$ @AhmedMasud Java 8 has methods that use ints as if they were unsigned. But you cannot declare a primitive that by itself is unsigned. See stackoverflow.com/q/25556017/1310566 \$\endgroup\$ – Simon Forsberg Dec 18 '16 at 17:33
  • 2
    \$\begingroup\$ The second possibility is not be valid if age is final, so it may not be a viable option. \$\endgroup\$ – James_pic Dec 19 '16 at 18:52

10 Answers 10

5
\$\begingroup\$
  1. You should throw exceptions in case of invalid values. As suggested by Simon Forsberg you can throw IllegalArgumentException or you can make your user-defined exception if you want to customise it.

  2. You should completely avoid this habit because in the question provided, you have a simple piece of code, but there are areas where you deal with complex data structures and values/database interactions. If you keep following this habit, someday you will initialise the value to be returned before checking things and later on get trapped by exceptions that you might not have thought could occur.

\$\endgroup\$
39
\$\begingroup\$

The biggest problem I see here is that you are printing a warning message to System.out and then using a default value for something which sounds like an Exception.

Why allow negative values at all?

if (initialAge < 0) {
    throw new IllegalArgumentException("Initial age cannot be negative, was specified as " + initialAge);
}
age = initialAge;

This way it is always up to the caller to pass a valid age instead of the method defaulting to zero which will bring other problems. It is better to have your program break early because of a bug than to have a warning message be printed out which may indicate a bug.

\$\endgroup\$
  • 2
    \$\begingroup\$ Side note: If you absolutely must choose between your options. Go with the if-else. I am not mentioning that in my answer because I don't believe that is the right approach. The right approach is to let the caller determine the default value if for some weird reason it is negative. \$\endgroup\$ – Simon Forsberg Dec 18 '16 at 17:41
  • \$\begingroup\$ Age is not valid, setting age to 0 And that 0 is not even a valid age either \$\endgroup\$ – Hanky Panky Dec 20 '16 at 10:40
  • \$\begingroup\$ @HankyPanky if you're newly born, it could be. \$\endgroup\$ – Simon Forsberg Dec 20 '16 at 11:15
  • \$\begingroup\$ Hello @SimonForsberg, Appreciate the response! The snippet I provided is part of my submission to a coding challenge. One of the requirements was to set any negative number inputted to 0. \$\endgroup\$ – Russ Wilkie Dec 20 '16 at 14:37
  • \$\begingroup\$ @RussWilkie That can still be done outside the Person constructor. \$\endgroup\$ – Simon Forsberg Dec 20 '16 at 15:12
4
\$\begingroup\$

You put the main functionality before the input verification. Either verify first

if(!foo) {
  throw new IllegalArgumentException("not foo");
}
doStuff();

or use an if-else with the main functionality first

if(foo) {
  doStuff();
} else {
  handleWrongFoo();
}

The ugliest part of your solution, to me, is doing something invalid, then going "no, wait".

do {
  doStuff();
} ormaybeif(!foo) {
  handleWrongFoo();
}
\$\endgroup\$
3
\$\begingroup\$

As others have pointed out, you should probably throw an exception here. A minor style point, is that if possible I usually put the main flow of the program in the if part with non-standard processing in the else part.

In this case, either:

if (initialAge >= 0) {
  age = initialAge;
} else {
   System.out.println("Age is not valid, setting age to 0.");
   age = 0;
}

or:

if (initialAge >= 0) {
  age = initialAge;
} else {
  throw new InvalidParameterException("Person(): invalid parameter. initialAge = " + initialAge)
}

I also prefer if (...) with a space between the if and the opening parenthesis. This helps distinguish if, when and other language statements from method calls, where there is no space.

\$\endgroup\$
  • 1
    \$\begingroup\$ which statement that goes where (normal case first vs. edge case first) is a bit opinion-based. The positive thing about throwing an exception in the normal if is that then you wouldn't need an else \$\endgroup\$ – Simon Forsberg Dec 19 '16 at 8:44
  • \$\begingroup\$ putting the main flow in if prevent from using elseif. I usually go like if(wrong){throw new Exception1} elseif(also_wrong){throw new Exception2} else{everything="OK"} \$\endgroup\$ – Dan Chaltiel Dec 19 '16 at 10:46
  • \$\begingroup\$ How about: if (OK) { doNormal(); } else if (wrongType1) { throw new Type1Exception(); } else { throw new OtherTypeException(); }. Not a major point, more a matter of style. \$\endgroup\$ – rossum Dec 19 '16 at 11:56
2
\$\begingroup\$

It depends on the situation but for the situation mentioned in your code not only it reduce performance but also it decrease your code readability. Performance issue is because of setting a variable two times in some cases. And as a code reader I wonder why you change a set variable again(For code readability).

\$\endgroup\$
  • 3
    \$\begingroup\$ I'd say that the performance issue is not really an issue, that's quite irrelevant. \$\endgroup\$ – Simon Forsberg Dec 18 '16 at 19:47
  • \$\begingroup\$ @SimonForsberg Hey simon, your response intrigues me. Why would performance not be an issue? Is this mainly because of the outlying issue in my example outweighs any performance issue(s) or would performance also not be relevant for similar coding examples? \$\endgroup\$ – Russ Wilkie Dec 21 '16 at 15:56
  • \$\begingroup\$ @RussWilkie It's because computers are so fast you will barely be able to tell the difference. Maybe if you do it a billion times you might have saved a few milliseconds. No matter if you set the value once or twice, it is still very cheap constant-time operation. \$\endgroup\$ – Simon Forsberg Dec 21 '16 at 17:17
1
\$\begingroup\$

I still use a no-else structure and find it cleaner than the conventional if/else for selected cases. This example doesn't avoid the else, of course, but it does illustrate the ordering I use: default, good case, fail case.

function Person(int initialAge) 
{
    age = 0 ; // initialise to the default value

    if (initialAge > 0)    // if passed a good value
    {
        age = initialAge ; // use it 
    }
    else  // but if passed-in value not good, complain
    {
        System.out.println("Supplied age not valid, age set to 0.");
    }
}
\$\endgroup\$
1
\$\begingroup\$

Others have already pointed out the flaws in this coding habit, however, there is an upside to it. If performance is important to you then this is supposedly a good coding practice.

Modern processors does a lot of prediction to help optimize execution. Every branch you add makes this prediction harder or impossible, which harms performance. An if-statement is one branch, the else-statement is another. If you skip the else-statement and preemptively do what you would otherwise do inside the else-statement before the if-statement then this will make prediction easier for the processor, thus improving performance. Of course, it depends on how expensive the operations inside the else-statements are. As long as it's just assigning values to a few variables then it's fine.

Source: My teacher in the Optimization course at The Game Assembly, where I study Game Programming.

I know some programmers will scoff at these kinds of "micro optimizations" as being unnecessary or pointless, but performance is very important in game development. If this piece of code is going to run thousands of times each frame then it is well worth doing this kind of optimization.

Edit: The performance gain might be affected by what language you're coding in. This course was held in C++, which is widely used in game development for having great performance.

\$\endgroup\$
-1
\$\begingroup\$

I would agree that this is a bad habbit althoug I always try to find alternatives to an if-else-statement.

My first attempt would be:

class Person {
    private final int age;

    /**
     *  Creates a new person with age 0.
     */
    Person() {
        age = 0;
    }

    /**
     * Creates a new person with age
     * @param age the age of the person. Must be <code>>0</code>
     */
    Person(int age) {
        assert age>0;
        this.age = age;
    }
}

My second attempt would constist of an interface and two classes:

/**
 * Anything with an age.
 */
interface HasAge {
    /**
     * @return the age
     */
    int getAge();
}



/**
 * A Person has name, gender, address etc
 */
class Person {
}


/**
 * A Person with a fixed age.
 */
class PersonWithAge extends Person implements HasAge{
    private final int age;

    PersonWithAge(int age) {
        this.age = age;
    }

    @Override
    public int getAge() {
        return age;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why would you implement an interface for this? Also, assert statements are usually ignored by the JVM unless you are running with assertions enabled (which you aren't in production) \$\endgroup\$ – Simon Forsberg Dec 19 '16 at 20:39
-1
\$\begingroup\$

As others have noted, there could be problems, but it depends on the context. There are cases where the pattern above could be primarily beneficial.

  1. There were mentions to use language supported exception handing, but a) not all languages have them or the coder may not know how to use them properly, b) exception handling may introduce too much overhead for the given scenario (just like there are cases where it could to the contrary improve performance).

  2. The pattern may slow run-time performance, but it may improve it. First, in some languages, you can get compilers to compile the else statement similar to the pattern or vice-versa; however, it does on first sight appear that pattern may slow things down, so a compiler may not do it and it may not remove it either. And not all language environments have compilers. This means there are cases where only by avoiding that pattern (or putting it in by hand) could you improve performance. Let's see for a moment some reasons why the pattern (without else) could improve performance. A: For interpretive scenarios, cutting back on the number of language tokens, all else being equal, would shave time off the clock. B: For various reasons, there are many cpus that perform worse generally the more branching that exists in the machine instructions. The reason is that oftentimes the pipelines have to be flushed when branches are taken. There are predictive branching algorithms embedded inside the cpu. In some cases these could even have a particular branch improve performance, but generally you want to cut back on the number of branches you have in tight loops as a way to increase performance (and the pattern above fulfills that requirement). Sometimes cutting out branches can be had for free, but, as with this pattern, you can incur a cost, such as extra times setting the variable. However, if the extra writes happen infrequently enough, those costs could be overcome by the decrease in branches. Generally, though, you leave that kind of testing until the end. Unless you anticipate otherwise for a given case, that pattern is more likely than not to worsen performance or more likely just have no practical performance effect one way or the other.

  3. Code readability: It could be cleaner to apply the pattern or not. It depends on the specific case. As was stated in the question, it does appear to cut back on the amount of code. That and having one fewer indentation layer could make a particular section of code look more organized and easier to follow.

  4. More correct program: As was stated in one of the suggested answers, you may prefer to find bugs as early as possible in the design stages; however, not all programming tasks have the time in the world. Under times stresses, sometimes programming with extra layers of protection can solve the needed problem quickest with fewest problems. The greater chance of introducing or hiding hard-to-detect bugs might just be deemed less important at the moment that avoiding the negative effects of other bugs. In the given case, making sure "age" is set in this section of the code might be very important and by setting it early and in a single place, you could avoid potential future missteps during the addition of extra or longer branches which fail in some part to guarantee "age" is set.

  5. Some of the complaints were against assumptions made about the code used in the question (what does "age" represent, what is the average case, etc), but at a more fundamental level, the question was about a general practice so those complaints are not complaints against the pattern.

  6. For code you are writing for your own use or where you are the only one managing the code, what suits you best is possibly just what might be best, when we consider your overall productivity.

  7. As we move into a world where robots and computers become more adept than humans at creative tasks and pose an increasing threat of one day enslaving humans, code written that simply fails to follow patterns or to use the features of a language to clarify intent may end up being a key defense for humans. I suggest as cheap insurance that some of this style of code be sprinkled here and there. For those times, I would follow the pattern for some of the code and not follow it in other places. [This may also come in handy if you fear being displaced in your job, but it may backfire. You have been warned.]

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview! Please take a moment to read over the how-to guide for answering questions. Specifically, please try to contribute something new in an answer and not simply borrow from all other answers. As an aside, I don't think performance or robots are of much concern in this question. \$\endgroup\$ – avojak Dec 21 '16 at 19:19
  • \$\begingroup\$ a number of points i mentioned were not mentioned elsewhere. in particular the discussion on performance and on how time constraints/avoidance of particular bugs can make this pattern superior (eg, it's clear in setting variable). I referenced other comments to add continuity and context (point/counterpoint) and I recognized their value but showed alternative scenarios exist where they may not apply. [whole post was on providing scenarios missed by other comments]. I still hope this comment is valuable to at least some other readers for the new observations it makes and the light touch at end. \$\endgroup\$ – Jose_X Dec 25 '16 at 13:09
-2
\$\begingroup\$

An example where this can go terribly wrong: you have multiple threads which may have access to a variable foo. Imagine a situation where you first set foo to the else-value in thread 1. Then the CPU gets handed to thread 2 which reads the wrong value from foo before thread 1 is able to set the correct one.

Of course there are a lot of ways to avoid this, but let's be honest - everyone makes mistakes. And when the mistake is using a slightly wrong value in a complex calculation, well, good luck debugging.

Bonus information: both C++ and Java (and other languages?) allow you to just omit the curly brackets around the else for a single statement, eg:

if (foo) 
    bar = 1;
else
    bar = 2;
\$\endgroup\$
  • 3
    \$\begingroup\$ Using an if-else statement does not make code less susceptible to threading bugs. Also, I strongly recommend against omitting curly braces - that can be an easy source of hard-to-find bugs if you're not careful. \$\endgroup\$ – avojak Dec 19 '16 at 4:14
  • 1
    \$\begingroup\$ @Drascam you can always do bar = foo? 1: 2;. Hurray for ternaries \$\endgroup\$ – A T Dec 19 '16 at 5:23
  • 2
    \$\begingroup\$ The code in the question is written in a constructor, the object is not available until the constructor has finished as long as it is not leaked from the constructor, which it isn't in this case. \$\endgroup\$ – Simon Forsberg Dec 19 '16 at 8:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.