6

This is a topic in one of our code reviews, I'd like more opinions.

Let's say I am writing a service that will allow me to insert a simple Person object into a database.

public class Person
{
   private String name;
   ....
}

We have a simple VerifyNotNull method throws IllegalArgumentException.

Which route would you take and why.

Option 1:

Verify not null in constructor of Person object.

public Person(String name)
{
     VerifyNotNull(name);//throws illegal arg exception if name is null
     this.name = name;
}


Option 2:

Allow Person to be constructed with null, verify not null on addPerson call.

public class PersonService
{
  public void addPerson(Person personToAdd)
  {
     VerifyNotNull(personToAdd.getName());//throws illegal arg exception
     //add code
  }
}

I don't like the idea of throwing Exceptions in constructors. To me option 2 feels right, I don't know how to explain or justify it though.

Is it acceptable to throws Exceptions in constructors?

Thanks for your help!

1
  • 1
    I'm not fond of throwing exceptions from an initializer either -- however, I'd much rather have this than an object with clearly "invalid" (immutable) state. One major problem/difference with Option #2 is that it might never pass through addPerson: the issue with the null name is thus only in relationship to the PersonService.
    – user166390
    Nov 29, 2011 at 17:36

7 Answers 7

12

The first approach is more fail-fast, which will increase the likelihood that you'll find the source of your bug more quickly. Think of it like this: if your error log starts telling you that a number of errors have been cropping up because you're trying to add people that have null names, you're going to want to know where those people's null names came from, right? Depending on how your system is structured, it's possible that the person was created miles away from the place where the person is getting added to the service. So you have no idea which of the four thousand places in your code is creating people without names.

So if I had to choose, I'd go with the first option.

Of course, it depends on your business model. If a person will a null name is a perfectly legal thing to create when you're in the data-entry phase, and it's not until you're getting ready to persist that person's information that you want to make sure it's passed validation, then that's a different story. In that case, you might even want to come up with a ValidatedPerson class that wraps Person, but indicates in a type-safe way that the addPerson method can only be called if someone has gone through the validation process, because the only way to create a ValidationPerson is through a specific validate method that checks the person's name.

6

I'd throw the exception in the Ctor. The main reasons for this are:

  • It doesn't even allow the creation of invalid Person objects.
  • You don't need to check if the Person object is valid at every point in your application where you use the Person object.
  • The exception is thrown closer to the code which is causing the invalid Person object to exist (e.g. its called right when you create the object and not much later in your code when who knows who has processed the object already).
  • Code will fail fast - which is a good practice.

If you are using IntelliJ you can also use JetBrain's @NotNull and @Nullable annotations, so the IDE will also give you a warning if you call the Ctor with a null argument. See http://www.jetbrains.com/idea/documentation/howto.html.

1
  • 1
    +1 I don't see any good reasons to NOT throw an exception in a constructor if you can't create a valid object. And it's certainly a much better alternative than having an invalid object around.
    – Voo
    Nov 29, 2011 at 17:44
3

Your choice is not limited by two options you listed.

  • It is entirely possible to guarantee object validation without throwing exception from constructor.

For example, one could use factory method to achieve that:

class Person {
    private final String name;
    private Person(String name) { // private constructor
        this.name = name;
    }
    public static Person newPerson(String name) { // factory method
        VerifyNotNull(name); // IAE not from c'tor, guaranteed check
        return new Person(name);
    }
 }

Another way, if one wants to avoid static methods, could be with Builder like below:

class Person {
    private final String name;
    private Person(String name) { // private constructor
        this.name = name;
    }

    /**
     * Usage: Person p = new Person.Builder(name).build();
     */
    public static class Builder {
        private final String name;
        public Builder(String name) {
            this.name = name;
        }
        public Person build() {
            VerifyNotNull(name); // IAE not from c'tor, guaranteed check
            return new Person(name);
        }
    }
}
5
  • 1
    So we've turned a simple new Person(name) into new Person.Builder(name).build() in the name of.. what exactly? What's the problem with throwing an exception from the constructor - I don't see any advantage of the second solution apart from it being rather overengineered.
    – Voo
    Nov 29, 2011 at 19:14
  • @Voo why don't you just search the web for something like java exceptions in constructor? As for me, few times I've experienced exception in constructor in action myself were enough to understand why some prefer to avoid it
    – gnat
    Nov 29, 2011 at 20:39
  • 1
    Apart from an interesting security attack which can (and has to be for other reasons anyhow) easily be circumvented and isn't applicable in 99.99% of all scenarios I don't get many interesting reasons, so go on and link me to some nice summary if you don't want to summarize your points yourself :)
    – Voo
    Nov 29, 2011 at 23:08
  • @Voo do you consider security breach harmless? As for my personal dislike, it comes from the fact that stack trace reports exception happening at <init> instead of usual method name. This may look minor when you sit comfortably staring at toy code in StackOverflow window class Test { public static void main(String[] args) { class Demo { Demo() { throw new IllegalArgumentException("demo"); } } new Demo(); } } But wait until it drops at your head in the middle of smoke-testing for some new piece of code
    – gnat
    Nov 30, 2011 at 6:07
  • No, init wouldn't surprise me at all - hey java bytecode 101 was useful after all - but since you get the correct line with the trace anyhow that's hardly a problem (Oh goodness it says line 10 in file xyz.java but it's the wrong method name! What should I do?). About the security breach: Not applicable in 99.999% of all situations (for one you have to add classes and code to the project, so why not just edit the existing code?), not harmless. If you write your own classloader you better know about it, but even then it's easily fixed.
    – Voo
    Nov 30, 2011 at 14:02
1

It is acceptable to throw the exception in the constructor. However, after years of experience, I found that checking for null arguments or varibles in constructor or method are not neccessary unless you have a way to recover the problem. If there is no recovery plan, then the program is bound to fail anyway. For best practice, you should make sure that the arguments given to constructor or method are not null (if null isn't accpetable) before passing them.

1

Two different approaches.

The first approach is fail fast - i.e. you get to find out that there is a problem as soon as possible, and the error shows exactly how the null got to be there (stack trace). It's preferable, all else being equal.

However that approach only works if you are sure that a person never ever needs a name. Is it possible that you might need to create a "placeholder" person, who exists temporarily to serve some function but doesn't actually need a name? If so, then you need to use the second approach. In general placeholder objects are a bad idea, so use the first if you can. But ultimately only you know your application.

0

The rule that the name cannot be null is business logic, and therefore belongs in the Person entity. So the constructor option is the better of the two. However, you may wish to use a Factory method (Evans):

public class Person
{ 
    public Person()
    {
       //Nothing much here.
    }

    public static Person Create(String name)
    {
        VerifyNotNull(name);//throws illegal arg exception if name is null
        Person person = new Person();
        person.name = name;
        return person;
    }

    ...
}
0

I would say that it's better to throw the exception inside of the service method.

The reason is that validation checks, in general, are often dependent upon business requirements specific to a particular application; the service layer is where most application-specific business logic resides (caveat: there are arguments against that; see also: "anemic domain model"). If you add the constraint inside of the Person class, then said class becomes less reusable/portable, since you may want to use the class elsewhere that allows a null name property inside that particular constructor.

1
  • I also agree that second option is better because it is more reusable than the first one. First option is violating Object Oriented Principles. Even factory method is better design than that. Jun 19, 2015 at 12:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.