4
\$\begingroup\$

I have a method in which I am accepting a String clientId and that has below requirements:

  • clientId can be a positive number greater than zero. But if it is negative number or zero, then throw IllegalArgumentException with a message.
  • clientId cannot be a null or empty string. But if it is, then throw IllegalArgumentExceptionwith a message.
  • clientId can be a normal string as well. For example - it can be abcdefgh or any other string.

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;    

public Builder setClientId(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;
    return this;
}

This code works fine. Is there any other better way to write above logic? I am using Google Guava preconditions class for this. I have lot of other methods as like above in which I am using Google Guava preconditions to do some sanity checking.

\$\endgroup\$
  • \$\begingroup\$ If you will do it for every set methods, your code will be very, very unreadable... Also your Builder knows too much about preconditions / logic. Let the consumer (for example database) throw an exception. \$\endgroup\$ – dieter Jan 26 '16 at 20:23
6
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.