The code looks very good.
Check your requirements to see if "00", "0x0", etc are supposed to be a valid client ids. Whatever the answer, write unit tests to capture the requirements and validate that you are meeting them.
As written, you are violating the single responsibility principle: this constructor is acting both as a coordinator (figuring out which checks to run) and as a calculator (computing whether or not the regular expression is a match). I'd probably leave it, because the pattern is so common.
You are also (possibly) sharing some of the validation code for the "general" string based id (not null, not empty), which the rules specific to ClientId (not '0', not a negative integer). If so, some refactoring is called for.
String.matches(regex) is just a wrapper around Pattern.matches(regex,input). Given that you know that the regular expression isn't going to change from one call to the next, you might create a single static instance of the Pattern so that you aren't compiling it multiple times. It does complicate the logic in the constructor somewhat
checkArgument(! pattern.matcher(clientId).matches(), "clientId must not be negative or zero");
I'd be more likely to do this if I moved the match calculation into a separate function/factory -- the performance gains are not likely to be measurable, and readability is important.
I would suggest that you consider is that your representation of the data, after it has passed all of its validation checks, should probably differ from the representation before the checks have been validated.
public class ClientId {
// final, because we don't want code to later change the id to
// something that isn't valid.
private final String clientId;
public ClientId(String clientId) {
checkNotNull(clientId, "clientId cannot be null");
checkArgument(clientId.length() > 0, "clientId can't be an empty string");
checkArgument(!clientId.matches("-\\d+|0"), "clientId must not be negative or zero");
this.clientId = clientId;
}
}
class Builder {
public Builder setClientId(String clientId) {
return setClientId(new ClientId(clientId));
}
public Builder setClientId(ClientId clientId) {
this.clientId = clientId;
return this;
}
}
Done this way, all of the code that interacts with a client id knows from the data type that the constraints on the data have already been verified.
Typically, when implementing a value type this way, you'll also want to replace the default implementations of equals() and hashcode() to make sure that you still have string like semantics available.
Less typically, you will find examples where the standard is to use marker interfaces to denote values.
public ClientId implements ValueObject<ClientId> { ... }
or even
public ClientId implements Id<ClientId> {...}
The marker interfaces will sometimes specify queries that the types must implement
public interface ValueObject<T> {
boolean sameValueAs(T other);
}
You provide a bit of extra work in the class implementation, and you get some clarity/readability in the consumer code.
In some domains, you may need to worry about the fact that clever attackers can capture a reference to an incompletely constructed value type. In that case, putting the data validation in the constructor won't suffice, you'll instead want to use a factory method to construct the object. For instance:
public class ClientId {
private final String clientId;
private ClientId(String clientId) {
this.clientId = clientId;
}
public static ClientId create(String clientId) {
checkNotNull(clientId, "clientId cannot be null");
checkArgument(clientId.length() > 0, "clientId can't be an empty string");
checkArgument(!clientId.matches("-\\d+|0"), "clientId must not be negative or zero");
// with the checks verified, we take advantage of the fact that this
// method has access to the private constructor that is inaccessible
// everywhere else.
return new ClientId(clientId);
}
}
Written this way, the checks happen before the call to the java.lang.Object constructor exits, which is a critical point in the lifetime of the potential reference.
