Exceptions

The code in the Modern modules of BoneJ should throw exceptions in a consistent manner. Exceptions related to user-input, I/O and other areas where exceptions are to be expected (and can potentially be recovered from), should be caught with a catch block (which should have as specific exception type as possible, so that it doesn't accidentally catch something we don't expect). The user should be informed of the error, and the stack trace logged with a call to LogService.trace(). This way the exception info can be viewed by selecting Edit > Options > ImageJ2... and setting SciJava log level to TRACE if need be. Normally the user shouldn't see these kinds of exceptions. See issue #82.

NullPointer, IllegalArgument and other exceptions that result from programmer errors should not be caught. Otherwise these bugs might go unnoticed. These exceptions should only be thrown if they otherwise wouldn't already happen in the scope, or when they add new information about valid behaviour or data. Some examples:

// Example 1
public static char initial(final String name) {
  // Throwing a NullPointerException would be redundant,
  // because the next line will throw it if name == null 
  return name.charAt(0);
}

// Example 2
public class Employee {
  private String name;

  public String getName() { return name; }

  public void setName(final String name) throws NullPointerException {
    // Exception is thrown, because otherwise there's no telling when the null name field
    // would cause trouble
    if (name == null)
      throw new NullPointerException("Name cannot be null!");
    this.name = name;    
  }
}

// Example 3
public void setRadius(final double r) {
  // Exception thrown, because it adds a new constraint for valid data
  if (r <= 0) 
    throw new IllegalArgumentException("Radius must be positive!");
  radius = r;
}

// Example 4
public enum Day {
    SUNDAY, MONDAY, TUESDAY, WEDNESDAY, 
    THURSDAY, FRIDAY, SATURDAY 
}

public String dayName(final Day currentDay) {
  switch(currentDay) {
      case SUNDAY: 
        return "Sunday";
      ...
      case MONDAY:
        return "Monday";
      default:
        // Exception thrown, because code has entered a bad path
        throw new RuntimeException("Somebody forgot to cover all valid cases!");
  }
}

If you expect your method to throw a certain kind of exception, you should test for it - even if you haven't explicitly programmed the method to throw it. Often it's enough to just test for specific exception types:

@Test(expected = NullPointerException.class)
public void testSetName() {
   final Employee employee = new Employee();
   employee.setName(null);
}

However if your method can throw the same type of exception for multiple reasons, you should use JUnit @Rule:

@Rule
public ExpectedException expectedException = ExpectedException.none();
...
@Test
public void testSetNameThrowsIAEIfEmpty() {
   expectedException.expect(IllegelArgument.class);
   expectedException.expectMessage("Name cannot be empty");
   final Employee employee = new Employee();
   employee.setName("");
}

@Test
public void testSetNameThrowsIAEIfNumbers() {
   expectedException.expect(IllegelArgument.class);
   expectedException.expectMessage("Name cannot have number");
   final Employee employee = new Employee();
   employee.setName("John 13 Smith");
}

As a rule of thumb exceptions should be thrown as soon as possible. For example, if you have a chain of methods, who all expect an integer argument to be positive, you should check for it in the first method called - which often is the public one. There's no reason to repeat the check in the private methods, because it's reasonable for them to assume that bad parameters have already been filtered out. However, it's the responsibility of each class to know what kind of arguments they can digest. Even if you know your utility method will fail if you call it with a negative number, you shouldn't check for it in your Command. Checking for exceptions "too early" is also redundant, and quickly makes your code verbose.

Note that these are guidelines, not hard rules. These guidelines are also far from complete. Ultimately the question of how to throw exceptions is up to the programmer. The most important thing is to aim for consistency. You don't want methods in one class to fail quietly if arguments are null, and in other to throw exceptions.

Code that migrates from BoneJ to the framework should follow the exception conventions presented there. For example, ImageJ ops don't need to check if their arguments are null, because if that's the case, the op matching fails, and the op never gets called.