Verifying a person's age

Question

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.

Answer 1

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.

Answer 2

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.

Answer 3

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();
}

Answer 4

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.

Answer 5

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).

Answer 6

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.");
    }
}

Answer 7

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.

Answer 8

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;
    }
}

Answer 9

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.]

Answer 10

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;