Setter error checking

Question

I have a class called Duration which has the attributes hours, mins, and secs. These attributes have to stay within acceptable ranges, so I implemented the following in their setters:

public void setHours(int newHours){
 if (newHours >= 0){
 hours = newHours;
 }
 else{
 throw new RuntimeException("Invalid argument values");
 }
}

and so on for the other attributes.

Is there a better design pattern I could be using here? Is this an appropriate use of the RuntineException?

• tonysdg: If newHours must be >= 0, why not use an unsigned int? Granted, this may lead to problems if negative numbers are provided -- but that would (in theory) move the error checking out of the setter and into the code that uses the setter.
• 200_success: This is a private method — could you also show the code that calls it? I suspect that this class is improperly designed.
• njzk2: @tonysdg java doesn't have those.
• tonysdg: @njzk2: Seriously? Never knew that (I'm mainly a C/Python guy). Huh -- I wonder what was behind that (awful, IMO) design choice.
• Brett Warren: @200_success Whoopsies, this is supposed to be public, will edit.

Answer 1

Input validation is a critical aspect of any reliable program. Your code is functional, but there are a few things to note.

First up, the RuntimeException is not a good error type to return, use an IllegalArgumentException instead (because it is an illegal argument). Note that IllegalArgumentException extends RuntimeException so it does not need to be explicitly checked/thrown.

Next, you should validate inputs before using the values - using a "Guard Clause". While your code technically does that, the logic looks like the validation is done after the hours are updated.

Finally, error messages should give the user/programmer an indication of what's needed to fix the problem. Your message should be more useful.

So, all things considered, I would do the code as:

private void setHours(int newHours) {
 if (newHours < 0){
 throw new IllegalArgumentException("Input hours must be positive value: " + newHours);
 }
 hours = newHours;
}

• Sharon Ben Asher: "Invalid positive hours value" for a negative value?
• rolfl: @SharonBenAsher - Yeah, maybe it's not expressed well, but I try to have error messages that express what the value should be, and then what the value is. The hours is expected to be positive, but the value supplied is negative. I'm part of a team/community where the above message would make sense because it's a "common practice", but I can see the confusion.
• Sharon Ben Asher: why not "value must be positive"?
• rolfl: Sure, changed ;-)
• Koekje: Aren't these also called guard clauses? Perhaps you could mention that :D

Answer 2

Adding to the great answer of rolfl, should you do input validation and use exceptions to guard against invalid arguments, I'd say it is really important to document those. RuntimeExceptions are generally not visible from the method declaration. So for example:

/**
 * Sets the hours to the given value.
 *
 * @param newHours the new value for the hours
 * @throws IllegalArgumentException when {@code newHours} is negative
 */
public void setHours(int newHours) {
 ....
}