6
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – tonysdg Jan 15 '18 at 19:38
  • 1
    \$\begingroup\$ This is a private method — could you also show the code that calls it? I suspect that this class is improperly designed. \$\endgroup\$ – 200_success Jan 15 '18 at 20:50
  • \$\begingroup\$ @tonysdg java doesn't have those. \$\endgroup\$ – njzk2 Jan 16 '18 at 2:32
  • \$\begingroup\$ @njzk2: Seriously? Never knew that (I'm mainly a C/Python guy). Huh -- I wonder what was behind that (awful, IMO) design choice. \$\endgroup\$ – tonysdg Jan 16 '18 at 13:42
  • \$\begingroup\$ @200_success Whoopsies, this is supposed to be public, will edit. \$\endgroup\$ – Brett Warren Jan 16 '18 at 16:05
9
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ "Invalid positive hours value" for a negative value? \$\endgroup\$ – Sharon Ben Asher Jan 15 '18 at 15:39
  • \$\begingroup\$ @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. \$\endgroup\$ – rolfl Jan 15 '18 at 15:41
  • \$\begingroup\$ why not "value must be positive"? \$\endgroup\$ – Sharon Ben Asher Jan 15 '18 at 15:44
  • \$\begingroup\$ Sure, changed ;-) \$\endgroup\$ – rolfl Jan 15 '18 at 15:54
  • 2
    \$\begingroup\$ Aren't these also called guard clauses? Perhaps you could mention that :D \$\endgroup\$ – Koekje Jan 15 '18 at 19:31
2
\$\begingroup\$

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) {
    ....
}
\$\endgroup\$

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.