Acquiring a data key

Question

I recently started using the builder pattern in one of my projects:

public final class DataKey {

    private final long userId;
    private final String uuid;
    private final String deviceId;
    private final int clientId;
    private final long timeout;
    private final FlowEnum flow;

    private DataKey(Builder builder) {
        this.userId = builder.userId;
        this.uuid = builder.uuid;
        this.deviceId = builder.deviceId;
        this.clientId = builder.clientId;
        this.timeout = builder.timeout;

        if (userId == 0 && uuid != null && uuid.isEmpty() && deviceId != null
                && deviceId.isEmpty()) {
            throw new IllegalStateException("You have to pass at least one"
                    + " of the following: userId, uuid or deviceId");
        }
        if (userId != 0) {
            this.flow = FlowEnum.USERFLOW;
        }
        else {
            this.flow = FlowEnum.DEVICEFLOW;
        }
    }

    public static class Builder {
        protected long userId;
        protected String uuid;
        protected String deviceId;
        protected final int clientId;
        protected long timeout = 200L;

        public Builder(int clientId) {
            this.clientId = clientId;
        }

        public Builder setUserId(long userId) {
            this.userId = userId;
            return this;
        }

        public Builder setUuid(String uuid) {
            this.uuid = uuid;
            return this;
        }

        public Builder setDeviceId(String deviceId) {
            this.deviceId = deviceId;
            return this;
        }

        public DataKey build() {
            return new DataKey(this);
        }
    }

    public long getUserId() {
        return userId;
    }

    public String getUuid() {
        return uuid;
    }

    public String getdeviceId() {
        return deviceId;
    }

    public FlowEnum getFlow() {
        return flow;
    }

    public int getclientId() {
        return clientId;
    }

    public long getTimeout() {
        return timeout;
    }
}

I also have an enum which determines whether userId or uuid/deviceId is being used. If userId is present, then the flow type is USERFLOW, otherwise it's DEVICEFLOW.

public enum FlowEnum {
    USERFLOW, DEVICEFLOW
}

While making keys using the Builder class, I will always pass clientId will pass at least one ID key from these - userId, uuid and deviceId. If by any chance, all the ID keys are missing, then I need to throw an illegal state exception.

I need to prioritize the IDs as well if we are passing multiple IDs instead of one then userId takes the priority first, then uuid and then deviceId.

Can a validation check be done in the build() method? Also, is it thread-safe?

DataKey keys = new DataKey.Builder(100).setUserId(1234L).setUuid("aaaa").setDeviceId("sssss").addTimeout(100L).build();

System.out.println(keys.getFlow());

Answer 1

DataKey

State- vs. Input validation:

In your constructor you do something not quite intelligent. You first set all the fields and then check them for integrity.

IMO you should check the input data for validity before using it with something. This allows you to extract a so-called "leading guard clause". Some condition before a block of code makes it fail.

In fact you could extract the validation from the DataKey to the Builder while you're at it. Compare:

private DataKey(Builder builder) {
    if(!builder.isValid()){
         throw new IllegalStateException(INVALID_BUILDER_MESSAGE);
    }
    //keep doing the stuff you do ;)
}

Or even sooner:

public DataKey build(){
    if(!this.isValid()){
        throw new IllegalStateException(INVALID_BUILDER_MESSAGE);
    }
    return DataKey(this);
}

then you could remove the validation "clutter" from your DataKey constructor. And your isValid() method would look like:

private boolean isValid() {
    return !(userId == 0 && uuid != null 
      && uuid.isEmpty() && deviceId != null
      && deviceId.isEmpty());
}

If-Statement vs Ternary operator:

    if (userId != 0) {
        this.flow = FlowEnum.USERFLOW;
    }
    else {
        this.flow = FlowEnum.DEVICEFLOW;
    }

You can shortcut this drastically by using a ternary operator. IMO this also makes your intent way more clear:

this.flow = (userId == 0) ? FlowEnum.DEVICEFLOW : FlowEnum.USERFLOW;

You can shorten this even further by adding a static import to your class:

import static fully.qualified.FlowEnum.*;

and then you get:

 this.flow = (userId == 0) ? DEVICEFLOW : USERFLOW;

But that one is personal preference, some people even see this as light obfuscation.

While we're at your enum. Currently it's no more than a glorified boolean. I will assume there could be more Flows, because if not you could just do:

this.isUserFlow = userId != 0;

Answer 2

I also have an enum which determines whether we are using userId or uuid/deviceId. If userId is present then flow type is USERFLOW otherwise flow type is DEVICEFLOW.

That, by itself, strikes me as suspicious. Your choice of variable names has me thinking that you are using the enum to switch on behavior, rather than providing two different flow objects.

I don't see any advantage to choosing the flow in the constructor, rather than choosing the flow in the Builder.

A common idiom which you could consider is a static factory method to create the builder.

public final class DataKey {
    public static DataKey.Builder builder(String clientId) {
        return new Builder(clientId);
    }
    ...
}

And then your example looks like

DataKey keys = DataKey.builder(100)
               .setUserId(1234L)
               .setUuid("aaaa")
               .setDeviceId("sssss")
               .addTimeout(100L)
               .build();

I find that chaining logic is much easier to read if you separate out the methods by line - but follow local conventions here.

You might also consider additional factory methods that clarify what's really going on (kirawan touched on this). You can tuck them all into the class definition itself - but another common choice is to create a separate class to hold your factory methods.

public class DataKeys {
    public static DataKey createFromUserId () {
        ....
    }

    public static DataKey createFromSessionId() {
        ....
    }

Those factory methods might return DataKey.Builder instead, depending upon how complicated things get.

public static class Builder {
    protected long userId;
    protected String uuid;
    protected String deviceId;
    protected final int clientId;
    protected long timeout = 200L;

    public Builder setUserId(long userId) {
        this.userId = userId;
        return this;
    }

This logic here is suspicious -- protected tells me that you intend that the Builder can be extended, but return this says that adding new set methods in the extended class isn't supported (the chaining logic doesn't work).

Getting chaining objects to work with inheritance is... complicated. If you aren't ready to put that work in now, you should probably declare the builder class to be final, and tighten the access modifiers on the member variables.

If you are ready to put in that work, then review Eamonn McManus; his essay does a pretty good job of explaining the hoops you need to jump through to replace return this; with something that works with inheritance.