1
\$\begingroup\$

I recently started using the Builder pattern in one of my projects.

Below is my builder class -

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;
        this.flow = (userId == 0) ? FlowEnum.DEVICEFLOW : FlowEnum.USERFLOW;
    }

    public static class Builder {
        protected final int clientId;
        protected long userId;
        protected String uuid;
        protected String deviceId;
        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(){
            if(!this.isValid()){
                throw new IllegalStateException(INVALID_BUILDER_MESSAGE);
            }
            return DataKey(this);
        }

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

    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 we are using userId or uuid/deviceId. If userId is present, then flow type is USERFLOW, otherwise flow type is DEVICEFLOW.

public enum FlowEnum {
    USERFLOW, DEVICEFLOW
}

While making keys using the above Builder class, I will pass clientId always, and I 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.

Below is the way I am making my keys -

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

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

Problem Statement:

As I mentioned above I have three kind of "ID's" - userId, uuid and deviceId. Now I need to prioritize the id's as well if we are passing multiple id's instead of one, then userId takes the priority first, then uuid and then deviceId. And then I need to make a URL appropriately.

Below are the combinations possible -

  • If we are passing all three IDs, then I will use userId and my URL will look like this -

    http://localhost:8080/data?id=keys.getUserId()&clientId=keys.getclientId()&key=userid
    
  • But if we are passing uuid and deviceId, then I will use uuid and my URL will look like this -

    http://localhost:8080/data?id=keys.getUuid()&clientId=keys.getclientId()&key=uuid
    
  • And if we are passing deviceId only, then I will use deviceId and my URL will look like this -

    http://localhost:8080/data?id=keys.getDeviceId()&clientId=keys.getclientId()&key=deviceid
    

I need to add an extra key parameter at the end of the URL which determines what ID key I have used and at the service layer, they will know the call is coming for userid or uuid and deviceid.

class Task implements Callable<String> {

    private final DataKey keys;

    public Task(DataKey keys) {
        this.keys = keys;   
    }

    public String call() throws Exception {

        // print whether it is USERFLOW or DEVICEFLOW?
        // below will print out which flow I am using
        System.out.println(keys.getFlow());

        String url = createURL();
        System.out.println(url);

        // execute the URL

    }

    // added the above algorithm of prioritizing id keys
    public String createURL() {
        StringBuilder url = new StringBuilder();
        String key = "userid";
        if (keys.getFlow().equals(FlowEnum.USERFLOW)) {
            url.append("http://localhost:8080/data?id=" + keys.getUserId() + "&clientId="
                    + keys.getClientId() + "&key=" + key);
        } else {
            if (keys.getUDID() != null && !keys.getUDID().isEmpty()) {
                key = "udid";
                url.append("http://localhost:8080/data?id=" + keys.getUuid() + "&clientId="
                        + keys.getClientId() + "&key=" + key);
            } else {
                key = "deviceid";
                url.append("http://localhost:8080/data?id=" + keys.getDeviceId() + "&clientId="
                        + keys.getClientId() + "&key=" + key);
            }
        }

        return url.toString();      
    }
}

I'm opting for code review to see whether we can simplify the above createURL logic if possible. Is there any improvements which I can do?

Or this prioritization can also be done in DataKey builder class instead of doing it in createURL method of Task class? May be I can use build method in my builder class for this?

\$\endgroup\$
1
\$\begingroup\$

My first comment is there is duplication in your createURL(). I find that as duplication is reduced, new abstractions start to avail themselves.. Or I just end up with code that's simpler and easier to read. Here's my take on your createURL():

public String createURL() {
    String key = null;
    String id = null;
    if (keys.getFlow().equals(FlowEnum.USERFLOW)) {
        key = "userid";
        id = Integer.toString(keys.getUserId());
    } else if (keys.getUDID() != null && !keys.getUDID().isEmpty()) {
        key = "udid";
        id = keys.getUuid();
    } else if (keys.getDeviceId() != null && !keys.getDeviceId().isEmpty()) {
        key = "deviceid";
        id = keys.getDeviceId();
    }
    return "http://localhost:8080/data?id=" + id + "&clientId=" + keys.getClientId() + "&key=" + key;      
}

I find the above to be clearer. My eyes immediately sees there is just one consistent URL format with two conditional parameters. In the original implementation, there were 3 separate URLs which I had to take the time to determine they were the same format.

Second comment is that perhaps the if logic shouldn't be a concern of URL creation and is the responsibility of a DataKey. This is for you to decide based on your design and your domain. As you suggested, move the conditional logic into the DataKey (or even into the Builder) and expose only the key and id:

private DataKey(Builder builder) {
    this.clientId = builder.clientId;
    this.timeout = builder.timeout;
    this.flow = (userId == 0) ? FlowEnum.DEVICEFLOW : FlowEnum.USERFLOW;
    if (this.flow.equals(FlowEnum.USERFLOW)) {
        this.key = "userid";
        this.id = Integer.toString(builder.userId);
    } else if (builder.uuid != null) {
        this.key = "udid";
        this.id = builder.uuid;
    } else if (builder.deviceId != null) {
        this.key = "deviceid";
        this.id = builder.deviceId;
    }
}

createURL() is now dumb expects a DataKey to provide the correct id and key:

public String createURL() {
    return "http://localhost:8080/data?id=" + keys.getId() + "&clientId=" + keys.getClientId() + "&key=" + keys.getKey();      
}
\$\endgroup\$
  • \$\begingroup\$ Thanks chashi. In my case, userId is long, it's not a string. uuid and deviceId, they are string. And in your example, you are assigning id = keys.getUserId(); which is compilation issue for me. \$\endgroup\$ – arsenal Jul 1 '14 at 20:13
  • \$\begingroup\$ I should have stated that the code is untested and likely to be filled with those types of minor errors. I knew you would correct whatever issues were you to take the advice from my post. :) Regardless, I'll edit the post. \$\endgroup\$ – chashi Jul 1 '14 at 20:15
  • \$\begingroup\$ I got an idea how I can improve my code for sure but what should I do in this scenario? Any thoughts? \$\endgroup\$ – arsenal Jul 1 '14 at 20:17
  • \$\begingroup\$ I see, so you just converted to a string. Now in your second suggestion, you created a new string called id and key in the Builder class right? and then the logic in createURL was moved to constructor of DataKey class? \$\endgroup\$ – arsenal Jul 1 '14 at 20:27

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.