7
\$\begingroup\$

I recently started using Builder pattern in one of my projects and I am trying to add some sort of validations on my Builder class. I have already asked a question, got some feedback and incorporated it into my code.

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 FlowEnum flow;

    private DataKey(Builder builder) {
        this.userId = builder.userId;
        this.uuid = builder.uuid;
        this.deviceId = builder.deviceId;
        this.clientId = builder.clientId;
        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 boolean userIdFlag = false;
        protected boolean uuidFlag = false;
        protected boolean deviceIdFlag = false;         

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

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

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

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

        public DataKey build(){
        if (userIdFlag) {
            if (userId <= 0) {
                throw new IllegalArgumentException(" USERID cannot be negative or zero");
            }                       
        } else if (uuidFlag && !userIdFlag) {
            if (uuid == null || uuid.isEmpty()) {
                throw new IllegalArgumentException("UUID cannot be null OR empty");
            }                   
        } else if (deviceIdFlag && !userIdFlag && !uuidFlag) {
            if (deviceId == null || deviceId.isEmpty()) {
                throw new IllegalArgumentException("DEVICEID cannot be null OR empty");
            }                   
        } else if (!userIdFlag && !uuidFlag && !deviceIdFlag) {
            throw new IllegalStateException("You have to pass at least one"
                    + " of the following: userId, cguid or udid");
        }           

            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;
    }
}

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 atleast 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.

Problem Statement:

As I mentioned above I have three kind of "ID's" - userId, uuid and deviceId. userId takes the priority first, then uuid and then deviceId. Now I need to do validations on all these three ID's with below rules -

  • Suppose if we are passing all the three ID's, then I need to use userId since userId takes the priority so I will check whether userId is negative or zero, if it is, then throw an IllegalArgumentException that userId cannot be negative or zero. It is fine, if somebody is passing uuid and deviceid as null string or empty string in this case.
  • Now suppose if we are passing uuid and deviceid only, then I need to use uuid since uuid takes the priority now as userId is not present so I will check whether uuid is null or empty, if it is, then throw an IllegalArgumentException that uuid cannot be null or empty. It is fine, if somebody is passing deviceid as null string or empty string in this case.
  • Now suppose if we are passing deviceid only, then I need to use deviceid since deviceid takes the priority now as userId and uuid are not present so I will check whether deviceid is null or empty, if it is, then throw an IllegalArgumentException that deviceid cannot be null or empty.

I'm opting for code review to see whether we can simplify or improve the above code basis on the above logic if possible. Is there any improvements which I can do? Do I need boolean flag to make this work?

I can wrap null/empty string and long check in a method in my Utilities class but apart from that, is there anything I can improve?

This is one way of making the keys -

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

or

DataKey keys = new DataKey.Builder(100).setUserId(1234L).build();

or

DataKey keys = new DataKey.Builder(100).setUserId(1234L).setUuid("asdfg").setDeviceId("htyui").build();

Update:-

There is one issue with your suggestion -

Suppose if I am making keys like this, then it will throw IllegalArgumentException, right? Since uuid and deviceid are null or empty but with my above rules, if userId is set and it is set with proper value, then it should not throw IllegalArgumentException. Meaning if userId is set, then I will validate userId to see whether it is <= 0 or not.

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

There is always a priority, userId takes the priority first, then uuid takes the priority and then deviceId takes the priority.

If userId is set, then I will validate userId, but if userId is not set and uuid is set, then I will validate uuid but if userId and uuid is not set, then I will validate deviceId.

\$\endgroup\$
  • \$\begingroup\$ @Vogel612 Thanks for edit Vogel. I will make sure that next time to add that as well. \$\endgroup\$ – arsenal Jul 1 '14 at 6:47
  • \$\begingroup\$ Does it make a factual difference whether you .setDeviceId("") or .setDevcieid(null) ? \$\endgroup\$ – Vogel612 Jul 1 '14 at 8:16
3
\$\begingroup\$

Flags and Flag-flags:

Everytime your Variable name includes flag, there should be a flag raised in your head. You then should go back and either extract a method or a class.

Usually a flag means you are trying to do two different things in one method and that's something you shouldn't. Instead you should extract a method for each separate behavior.

Instead of these boolean flags you should initialize them to null and just check if they are set when you validate.

//note that I changed the type of userId to the wrapper class Long
protected Long userId = null;
protected String uuId = null;
protected String deviceId = null;
protected final int clientId;

In your validation you can then just check for null or empty / 0 and throw an IllegalArgumentException in case that happens. then you don't even need that whole if-else swapping.

Repeatedly throwing exceptions:

You repeat your throwing of IllegalArgumentException in each and every block you have defined in your if-statements. Instead you should extract your throwing of IllegalArgumentException into a helper method:

private boolean throwOnNullOrEmpty(String value, String name) {
    if(value == null || value.isEmpty()) {
         throw new IllegalArgumentException(name + " may not be NULL or EMPTY");
    }
    return true;
}

What you are really doing in your validation:

Some pseudocode to show what exactly you are doing:

IF userId is set
    VALIDATE userId (input)
ELSE IF uuid is set
    VALIDATE uuId (input)
ELSE IF deviceId is set
    VALIDATE deviceId (input)
ELSE
    THROW IllegalStateException

Usually it is recommended to fail as early as possible. This means, you shouldn't even allow setting of null (or empty / <= 0) values to userId, uuid and deviceId. You can then proceed to drop about all of your validation except:

IF userId not set AND uuId not set AND deviceid not set
    THROW IllegalStateException

Accordingly you should change your setters to:

public Builder setUserId(long userId) {
    if(userId <= 0) {
        throw new IllegalArgumentException("userId may not be zero or less");
    }
    this.userId = new Long(userid);
    return this;
}

public Builder setUuid(String uuId) {
    throwOnNullOrEmpty(uuid, "UUID");
    this.uuId = uuId;
    return this;
}

public Builder setDeviceId(String deviceId) {
    throwOnNullOrEmpty(deviceId, "DEVICE ID");
    this.deviceId = deviceId;
    return this;
}

and then your build-method becomes:

public DataKey build() {
    if(userId == null && uuId == null && deviceId == null) {
        throw new IllegalStateException("You need to set at least one of userId, uuid or deviceId");
    }
    return new DataKey(this);
}
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your suggestion again. I like your wrapper approach of using Long but there is one issue with your example. I have updated the scenario for that in my question. Basically, if userId is set, no matter what is uuid and deviceId, I will validate only userId. But if userId is not set and uuid id is set, then I will validate uuid only and then if deviceId is set, then I will validate only deviceId. Since userId takes the priority first, then uuid and then deviceId. \$\endgroup\$ – arsenal Jul 1 '14 at 15:43
  • \$\begingroup\$ .. With your suggestion if proper valid userId is set and by mistake somebody has set empty or null string for uuid and deviceId, then it will throw an exception. \$\endgroup\$ – arsenal Jul 1 '14 at 15:43
  • \$\begingroup\$ @webby an exception is very unspecific. What kind of exception and where?? \$\endgroup\$ – Vogel612 Jul 1 '14 at 16:04
  • \$\begingroup\$ Meaning, suppose if I am setting userId which is proper userId, greater than 0 but if I am setting, uuid and deviceId empty or null string, then it should not throw IllegalArgumentException. In your suggestion, it will throw an IllegalArgumentException right? Since in my case, userId has the highest priority, then uuid and then deviceId. So if userId is set, then I will do validation only on userId but if uuid is set only not userId, then I will do validation only on uuid but if userId and uuid is not set both, then I will do validation on deviceId. \$\endgroup\$ – arsenal Jul 1 '14 at 16:11
  • \$\begingroup\$ By exception I meant, IllegalArgumentException. \$\endgroup\$ – arsenal Jul 1 '14 at 16:14
1
\$\begingroup\$

Don't use flags for this. If you can use Java 8, you can use Optional (there are libraries with similar types for older Java versions, e.g. FunctionalJava).

But there is an even better way: You can use generics in order to insure that all required fields are set. This is called the "type-safe builder pattern".

The easiest implementation (an example for using "phantom types") is this: http://michid.wordpress.com/2008/08/13/type-safe-builder-pattern-in-java/

I would recommend a slightly improved version based on the following Scala code here: http://dcsobral.blogspot.de/2009/09/type-safe-builder-pattern.html

I translated this version to Java in my blog post http://dgronau.wordpress.com/2014/05/07/typsicheres-builder-pattern-mit-java/ (sorry, it's in German).

\$\endgroup\$
  • \$\begingroup\$ The pattern in itself is very nice, but unfortunately this does not solve the problem. You would have to be able to pass an Initializer<WITH<int>, WITH<AT_LEAST_ONE_OF<Long, String, String>> The main problem is, that there is one of three minimum required, but more are possible \$\endgroup\$ – Vogel612 Jul 1 '14 at 12:29
  • \$\begingroup\$ In that case the first link with the TRUE/FALSE version will work for you: Just use one common generic parameter for all three "at-least-one" fields. \$\endgroup\$ – Landei Jul 1 '14 at 18:50

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.