When a class represents a property that might be invalid, how should the validation be done?

Question

I have a Product class which has among others an attribute Ean13 that encapsulates an EAN13 code.

Here is a prototype of the Product class:

@Entity
@Table(name = "tb_produtos")
public class Product implements Serializable {


    public Product() {
    }


    @Id
    @GeneratedValue
    private Integer id;

    @ManyToOne
    @JoinColumn(name = "ID_FABRICANTE")
    private Manufacturer manufacturer;

    @Column(name = "DESCRICAO")
    private String description;

    @Column(name = "URL")
    private String url;

    @Embedded
    private Ean13 ean;

    @Transient
    private Keywords keywords;

    ... getters and setters.
}

At first, I implement the EAN class as follows:

@Embeddable
public class Ean13 {
    private static final RuntimeException notValidEanException = new RuntimeException("NOT VALID EAN CODE");

    @Column(name = "ean_code", nullable = true, length = 13)
    private String code;

    public Ean13() {
    }

    public Ean13(String code) {
        validate(code);
        this.code = code;
    }

    private void validate(String code) {
        if (code == null || code.length() != 13) {
            throw notValidEanException;
        }
        if (!CharMatcher.DIGIT.matchesAllOf(code)) {
            throw notValidEanException;
        }
        String codeWithoutVd = code.substring(0, 12);
        int pretendVd = Integer.valueOf(code.substring(12, 13));
        int e = sumEven(codeWithoutVd);
        int o = sumOdd(codeWithoutVd);
        int me = o * 3;
        int s = me + e;
        int dv = getEanVd(s);
        if (!(pretendVd == dv)) {
            throw notValidEanException;
        }
    }

    private int getEanVd(int s) {
        return 10 - (s % 10);
    }

    //mover estes metodos para outra classe. 
    //TODO: Java 8. 
    private int sumEven(String code) {
        int sum = 0;
        for (int i = 0; i < code.length(); i++) {
            if (isEven(i)) {
                sum += Character.getNumericValue(code.charAt(i));
            }
        }
        return sum;
    }


    private int sumOdd(String code) {
        int sum = 0;
        for (int i = 0; i < code.length(); i++) {
            if (!isEven(i)) {
                sum += Character.getNumericValue(code.charAt(i));
            }
        }
        return sum;
    }


    private boolean isEven(int i) {
        return i % 2 == 0;
    }

    @Override
    public String toString() {
        return code;
    }
}

As you can see, the way I implement the constructor throws a RuntimeException when the object is instantiated with an invalid code.

An explanation about the Product class:

In this particular case I will use this class to hold information about products in a crawler application, that crawl few web sites and collect information about these products. One of this information is the EAN code. In this case the product and EAN object will by instantiate in a service class. Some EAN codes the application grabs from the web site are not valid EAN codes (can be another code, or some other string). At first I want to save this products without any code until I created a way to store it. So at first I have to validate this EAN code before persist it.

As you can see in the code above, I implement the validation in the method 'validate' that is called by constructor. In the way it is implemented, the validation will work more or less as follow:

Product p = new Product ();
try {
    Ean13 e = new Ean13("somecode");
    p.setEan13 (e);
} catch (InvalidEanCodeRuntimeException e) {
    //Log invalid ean code and product information. 
}
persist(p);

But there would be other ways to validate it.

I can make implement a public method in the Ean13 class as follow:

Product p = new Product ();
Ean13 ean = new Ean13 ("somecode");
if (ean.isValidEan ()){
    p.setEan13(ean);
}

or it could still be done as follows, putting the validation in an external util class:

Product p = new Product ();
String somecode = "somecode";
if (CodeUtil.isValidEan13 (somecode)){
    p.setEan13(new Ean13(somecode));
}

Certainly there are many other ways to implement it would work. But I would like to implement this validation in the most correct way, clean and elegant as possible and / or promote a discussion on ways to implement this kind of validation.

Answer 1

It depends on what your program is going to do with those objects. For example, if you are implementing a bar code scanner program, and in case of a scan error, the requirement is to write the (invalid!) code the scanner has detected into a log file, or display it to the user, I can imagine a scenario where is makes sense to construct and process Ean object even if it is invalid. If that is your case, it should be obvious you need an isValid method for something along the lines of:

  Ean eanCode;
  while(true)
  {
      eanCode = new Ean(ScanCode());
      if(eanCode.isValid())
         break;
      LogScanError(eanCode);
  }

However, if your program always expects Ean objects to be valid, and there is absolutely no need for invalid objects, do the validation in the constructor, and throw an exception in case it fails. The example above might be implemented like

  Ean eanCode;
  bool scanOk=false;
  while(!scanOk)
  {
      String code = ScanCode();
      try
      {
          eanCode = new Ean(code);
          scanOk=true;
      }
      catch(ValidationException ex)
      {
          LogScanError(code);
      }
  }

This second variant has IMHO a light "code smell", since it uses exceptions for control of flow, which is often seen as a bad practice.

Thus I would prefer the isValid variant if you need the "invalid" Ean codes in more than one place in your program in a sensible manner, and the "constructor variant" if invalid Ean codes are a rare exception and their usage can be can be circumvented by implementing some code like above.

Answer 2

Well there is always a school of though that you should always instantiate your objects in a valid state, so your solution is actually a best practice. If you make your object immutable you will find it very hard to miss things up that way.

Moving validation outside of the constructor could cause a trouble if you forget to call it. Other developers might believe that the object was created successfully through its constructor and start using it directly.

On the other hand making the isValid() method public might be a good idea to reuse validation functionality of the EAN code itself (in the UI for example).

Answer 3

Looking at your code, I would suggest moving the validation to the constructor. It can be considered bad practice to have too much logic in the constructor, though, so if you're doing a lot of work then I would recommend looking at the factory pattern.

You can have a private constructor and a public static Create method with a String parameter which will throw if it isn't valid and return the object that you expect.

Ean ean = Ean.create("123");

This makes more sense, as it is odd behaviour for a constructor to fail and have no object.