Verifying a person’s age

Posted on

Problem

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.

Solution

  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.

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.

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

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.

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

Leave a Reply

Your email address will not be published. Required fields are marked *