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

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.(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 c

StackExchange Code Review Q#115590, answer score: 5