Validating Arguments in Constuctor

Question

In general, I like to favour immutability where possible and not allow objects to be created in an invalid state or get into an invalid state. Sometimes though it feels like it might go too far.

Example: I have a User domain object (JPA/Hibernate entity). I only allow the object to be constructed in a valid state, all required fields must be used to instantiate the object and cannot be NULL. On bigger objects, this can sometimes make the constructor look noisy with lots of validation going on. In this case, I am only validating the arguments are not null. I'm not checking to make sure the user name is between 5 - 30 characters, or any other rules that might go with the class properties.

Is it reasonable to do the following? Would it be sensible to take it a step further and ensure the arguments also meet any length, min, max, format validation? Or is making an argument as @Nonnull good enough?

In cases where there are setters, the same validation logic would need to be present as well.

@Entity
@Table(name = "user")
public class User {

  @Id
  @GeneratedValue(strategy = GenerationType.AUTO)
  private Long id;

  @NotBlank
  @Column(name = "user_name", unique = true)
  private String userName;

  @Column(name = "password")
  private String password;

  @Email
  @NotBlank
  @Column(name = "email")
  private String email;

  @Column(name = "locked")
  private boolean locked;

  @Enumerated(EnumType.STRING)
  @Column(name = "role")
  private Role role;

  @ManyToOne(optional = true, fetch = FetchType.LAZY)
  @JoinColumn(name = "dealer_id", referencedColumnName = "id")
  private Dealer delaer;

  @OneToMany(fetch = FetchType.LAZY, mappedBy = "user")
  private List<EventLog> events;

  protected User() {
    this.events = new ArrayList<>();
  }

  /**
   * Constructor for User.
   *
   * @param userName Username of the user
   * @param password Encrypted password
   * @param email Email of the user
   * @param locked Determines if user account if locked or not
   */
  public User(@Nonnull String userName, @Nonnull String password, @Nonnull String email, boolean locked, @Nonnull Role role) {

    this();

    Preconditions.checkArgument(userName != null);
    Preconditions.checkArgument(password != null);
    Preconditions.checkArgument(email != null);
    Preconditions.checkArgument(role != null);

    this.userName = userName;
    this.password = password;
    this.email = email;
    this.locked = locked;
    this.role = role;
  }
}

Answer 1

I also like to favor immutability. However, in some cases, immutability has a performance drawback (if objects need to be modified often, immutability means new object must be constructed per every modification, causing garbage collection overheads). So consider carefully the performance vs easy-to-verify-correctness aspects of your software. The hot spots of your program should use mutable objects for performance reasons.

You are doing things in exactly the same manner I would do. It is much better to check in constructor that the argument is not null rather than risk having a NullPointerException later when the true source of the null pointer is hard to trace.

Do add checks for length, etc. if you use a length-limited varchar data type for these fields in the database.

About the only check I would omit here is password strength check. Obviously, you should have a check for apparent password entropy, but the calculation of such entropy could be so time-consuming that I would add it elsewhere. Don't forget to put the password entropy check elsewhere, though! It is very important!

Answer 2

Is hard to say with so little context. But, look at it from this point of view.

Is it reasonable to do the following? Would it be sensible to take it a step further and ensure the arguments also meet any length, min, max, format validation?

It would be consistent. If the missing constraints validation results in a data integrity violation, why to check only the 50% of the them?

Moreover, if we instantiate the User by hand, by the time the Java Bean Validations comes into play, the instance is already in an invalid state.

Due to the public constructor, I assume that we could initialize invalid users from any place. You will have to decide whether it's a problem or not.

In cases where there are setters the same validation logic would need to be present as well.

Yes. however, if you make them final you could call the setters from the constructor, what allows you to place the validations in a single location.

In addition to juhist's answer, the password format validation is still possible where it's right now.

Java Beans Validations provide us with built-in validations constraints such as @Pattern or allow us to build our own Custom constraints.

Answer 3

Execute the validation in the constructor may make it difficuilt to do unittesting because it forces you to create fully valid Objects every time even if it is not be necessary for a certain unit-test.

Example: if you want to test the function Customer.isOlderThan18() (i.e. the Customeris allowed to buy cigaretts). Full valid would require that your test Customer must get a valid-complicated password. This makes testing unnecessary comlicated.

If you realy want immutable objects you can execute the validation in a builder.

Often there are multibe different business defintions of valid: isAllowedToLogin (username, password), canBuyCigaretts(age), canDoOrders(has enought money) ....