Hold and validate an EAN13 code I've created a class that holds an EAN13 code. This class is an attribute of the Product class.

In the Ean13 class, the string that represents the EAN code is passed as a parameter to the constructor and the validation is performed in the constructor. I created some utility methods in that class, like sumEven, sumOdd, and isOdd, but I'm not sure if would be better to move it to a separated utility class.

@Embeddable
public class Ean13 {
    private static final RuntimeException NOT_VALID_EAN_EXCEPTION = 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 NOT_VALID_EAN_EXCEPTION;
        }
        if (!CharMatcher.DIGIT.matchesAllOf(code)) {
            throw NOT_VALID_EAN_EXCEPTION;
        }
        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 NOT_VALID_EAN_EXCEPTION;
        }
    }

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

    private int sumEven(String code) {
        int sum = 0;
        for (int i = 0; i &lt; 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 &lt; 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;
    }
}





@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;

    public Integer getId() {
        return id;
    }

    public void setId(Integer id) {
        this.id = id;
    }

    public void setCode (String code){
        ean = new Ean13(code);
    }

    public Manufacturer getManufacturer() {
        return manufacturer;
    }

    public void setManufacturer(Manufacturer manufacturer) {
        this.manufacturer = manufacturer;
    }

    public String getDescription() {
        return description;
    }

    public void setDescription(String description) {
        this.description = description;
    }

    public String getUrl() {
        return url;
    }

    public void setUrl(String url) {
        this.url = url;
    }

    public Keywords getKeywords() {
        return keywords;
    }

    public void setKeywords(Keywords keywords) {
        this.keywords = keywords;
    }


}
Don't pre-generate exceptions

On the exception in your original code: It's usually not a good idea to pre-generate exception instances. The stack trace is filled in by the constructor of Throwable, not by the throw statement, as the following code snippet demonstrates:

public class StackTrace {
    private static final Throwable t = new Throwable();
    public static void main(final String... args) throws Throwable {
        throw t;
    }
}


When you pre-generate exceptions, you will not be able to tell from the stack trace which call-tree caused the exception. In your case, you would instead see the call-tree that loaded the class:

$ java StackTrace 
Exception in thread "main" java.lang.Throwable
        at StackTrace.&lt;clinit&gt;(StackTrace.java:2)


In Java, pre-generated exceptions are usually only found in environments where the stack trace is unavailable by nature, like Java Card Classic.

Consider converting your String to int[] first.

Normally I'd stick to Strings.
However, the case of an EAN13 is special.
The nature of an EAN13 is to be a 13 digit code.
Converting your 13 character String to an int[13] first makes this aspect of the semantics of an EAN13 more obvious and simpler.

You could then use constants like private static final int CHECK_DIGIT_INDEX = 12 to make it more obvious how an EAN13 is parsed. Consequently int pretendVd = Integer.valueOf(code.substring(12, 13)); would become int pretendVd = ean13digits[CHECK_DIGIT_INDEX];

Use names which are easy to understand, i.e. communicate intent

I could not find the term "pretend" resp. pretendVd in whatever documentation about EAN13 is available to me. I would rather call it checkDigitFromCode. The variable name dv is totally surprising, as I would at least have expected it to be vd. But I think calculatedCheckDigit or checkDigitFromCalculation would be better names. Even if variables have local scope, if there are multiple variables of the same type with different semantics, I'd rather be explicit about the intent of a variable and thus give it a name which communicates the intent well.
The worst name of all is me: The code multiplies odds and then the name is me.

Method length

The method validate(String) is quite long, and it can be split into two methods as it performs validation on two levels. First it validates whether the code actually really is 13 digits. Second it validates whether the checksum digit of the 13 EAN13 digits is correct. So, two more methods could be extracted from validate(String).

Validation Framework

This is definitely far from mandatory, it's just food for thought and showing possibilities. If you use a persistency framework like a JPA implementation, or a dependency injection framework like Spring, or even both, or something similar, you might want to consider the usage of declarative validation via annotations using the javax.validation API.

Some people favor separating model and validation always. I wouldn't go that far, and I'll try to explain that with an exaggeration: Tomorrow they'll make 6 classes, one for the instance fields, one for the constants, one for the construction (factory), one for the validation, one for the instance methods and one for the static methods. Where should this stop? The SRP doesn't say that "Every software entity should do just one thing". It comes with a clause, and this clause is important: "Every software entity should do just one thing on its level of abstraction". But it also means something else: Ideally, every software entity should have just one reason to change, and a change should ideally affect only one software entity.

If the validation only changes together with the model, it might be more prudent to keep validation and model together. If the validation is to a large extent independent of the model, it might be wiser to keep them separate.

Thinking about coupling, cohesion and interfaces also helps. If the validation is so loosely coupled, i.e. decoupled from the model that it checks not a specific model but an abstract model, resp. could be reused for different similar models, it certainly should be separate. If the validation is very tightly coupled to the model, it should be more cohesive with the model. And if you insert methods in the model to interface with the validation which are only for the validation and no other user in the system, you have strong arguments for keeping the validation inside the model class.

Welcome to engineering, there's no perfect solution, it always depends... ;)

Properly declare static methods.

Your methods getEanVd(int), sumEven(String), sumOdd(String) and isEven(int) never access any instance fields and thus could all be declared static.

Because they are private methods, it could actually make you wonder why you have a parameter String code and an instance field String code with identical content, which leads to:

Consider swapping validation sequence in the constructor

I think I guess your intention of first validating, then constructing the object with the sequence validate(code); this.code = code; in the constructor. You do not want the object to be constructed before it's validated.

However, consider a few things.

The construction is not really completed until the constructor returns. Whether you throw an exception soon or late in the constructor doesn't matter from a black box perspective. It also doesn't matter from a performance perspective, as an exception is a bad case path, not a happy path. Usually the performance of bad case paths doesn't matter.

However, it matters from a whitebox correctness perspective. Someone might look at the methods sumEven(String code) and sumOdd(String code) and realize that they're checking the very same code which would eventually be stored in the field, and thus turn the signatures into sumEven() and sumOdd(). Compiles still fine, but breaks with NullPointerException because of accessing still null field code before it was assigned to a (potentially) non-null value in the constructor.

Therefore, given a choice, i.e. when the field initialization is as trivial as simply assigning parameters to fields, I'd initialize fields first, as that leads to more robust code: It will allow subsequent private instance methods to access the fields even when they are called from the constructor.

Throwing the correct exceptions

The code part if (code == null) { throw NOT_VALID_EAN_EXCEPTION; } will be very difficult for developers to debug when it happens. Developers will assume that code was a String which was not an EAN13, but they will not expect that code was null, because the generally accepted behavior and practice is that if you pass null and null is not allowed you get nothing else but a NullPointerException. Getting a different exception than NullPointerException in case of a not-allowed null is very unexpected and can make developers waste a lot of debugging time staring at their code mislead into totally false assumptions.

In general, when null is not allowed, I'd not check, I'd simply let the code crash wherever it crashes. Whether the first check in validate(String) throws NullPointerException or whether some later part like code.substring() throws NullPointerException doesn't matter, the stack trace will always show the essential part which is that it was thrown somewhere in validate() called from Ean13().