2

I have a factory class, which makes Order objects, which in turn create Item objects. These 3 classes are bundled in a single package with no other classes. Each item has an "ItemMode" attribute. I created excessive null checks to achieve a defensive programming style. While I am confident with this coding style, even though it can be a bit verbose, I'm now doubting this approach because of the following reasons:

  • If the amount of created objects and the amount of arguments that need to be validated increases, that's a whole lot of code that must be run. Could this could performance issues? If so, are we talking about nano-, milli- or plain seconds?
  • Considering only the factory is able to instantiate Order and Item objects, we could theoretically skip the redundant if==null checks in the other classes. I know no factory can be created with a mode that is null, so no Orders or Items could be created with a mode that is null. However, I know this because I wrote the code. This is not a priori clear for other developers on the project.

Would you keep the redundant checks or not?

I first made the Item class:

class Item
{
   //Constructor
   protected Item(ItemMode mode)
   {
       if(mode == null)
       {
           throw new IllegalArgumentException("mode should not be null.");
       }
   }

   ...
}

Then the Order class:

class Order
{
   //Constructor
   protected Order(ItemMode mode, ...)
   {
       if(mode == null)
       {
           throw new IllegalArgumentException("mode should not be null.");
       }
   }

   ...

   public void methodThatMakesSomeItems()
   {
        ...
   }
}

And lastly the factory class:

class Factory
{
   //Constructor
   public Factory(ItemMode mode, ...)
   {
       if(mode == null)
       {
           throw new IllegalArgumentException("mode should not be null.");
       }
   }

   ...

   public void methodThatMakesSomeOrders()
   {
        ...
   }
}
user1884155
  • 3,616
  • 4
  • 55
  • 108
  • 1
    In this case, I would lose the redundant checks. Since you've made the constructors protected, you don't need to worry as much about other people re-using your code badly. If you do decide to keep the null checking, I would *strongly* recommend using something like `org.apache.commons.lang.Validate.notNull(variable, "error message");` (or writing your own helper method) to reduce the code clutter. – azurefrog Oct 31 '14 at 15:04
  • I code validation in only two places. When data is entered into the application via the UI (web pages, restful calls, soap, reading in files, etc...) or inside of domain object factories. Everywhere else, I don't worry about it much. The reason for checking in domain object factories is because some objects have complex identity issues. Meaning what makes this object unique and/or valuable to the business? I don't believe you will perceive a performance hit (you might in a realtime system). I would then remove all other null checks from your code, but have global exception handling. – hooknc Oct 31 '14 at 15:12
  • Could you elaborate on the following sentence: "but have global exception handling". Do you mean each controller (assuming MVC) should have a try catch wrapper around everything, so that no matter what exceptions happens, the controller caatches it and acts accordingly? – user1884155 Oct 31 '14 at 15:27
  • @azurefrog Excellent suggestion! It is indeed more readable to have 5 lines of "notNull" instead of 25 (!) lines of "if, curly brace, throw exception, end curly brace, new line." – user1884155 Oct 31 '14 at 15:29
  • 1
    I would advise to check out the @NotNull annotation style. That makes your code readable and your code can be checked and validated. See also here: http://stackoverflow.com/q/4963300/461499 – Rob Audenaerde Oct 31 '14 at 15:35
  • @user1884155 that is close to what we do for 'global exceptions handling'. We actually use spring's exception resolver and a java servlet filter. The exception filter catches all possible exceptions and email/logs them to/for us and returns a generic 'something went wrong' page if possible. The spring exception resolver is pretty sweet and will show different pages for different exceptions. For example we use the spring resolver to catch 'object not found' exceptions and then show a generic 404 page. – hooknc Oct 31 '14 at 21:11

0 Answers0