4
\$\begingroup\$

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());
\$\endgroup\$
  • \$\begingroup\$ Personally I would opt for a factory method pattern as the Builder class does very little. There would be two static factory methods - createFromUserId(...) and createFromDeviceId(...). The code would be much simplified, and the IllegalStateException could never occur. \$\endgroup\$ – kiwiron Jun 29 '14 at 9:10
  • 1
    \$\begingroup\$ You cannot assign values to final fields outside of the initializer/constructor. Does this compile? \$\endgroup\$ – David Harkness Jun 29 '14 at 9:17
  • 1
    \$\begingroup\$ @kiwiron make that an answer.. you'd have my upvote. \$\endgroup\$ – Vogel612 Jun 29 '14 at 12:19
  • \$\begingroup\$ @DavidHarkness : That was a silly copy paste mistake from me. Edited the question with right code.. \$\endgroup\$ – arsenal Jun 29 '14 at 14:15
  • \$\begingroup\$ @kiwiron Can you provide an example for that using factory method pattern as the Builder class? \$\endgroup\$ – arsenal Jun 29 '14 at 14:16
5
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Thanks for the help. Now my code is looking pretty clean. I also have similar question here basis on this same question. See if you can help me out there if possible. \$\endgroup\$ – arsenal Jul 1 '14 at 2:29
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for suggestion. In my case, I can still pass all the three different ID's userid, uuid and udid altogether so how this case will be handled if I go with factory method pattern? \$\endgroup\$ – arsenal Jun 29 '14 at 14:58

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.