Skip to main content
added 72 characters in body
Source Link
Vogel612
  • 25.3k
  • 7
  • 58
  • 141
//note that I changed the type of userId to the wrapper class Long
protected Long useriduserId = null;
protected String uuiduuId = null;
protected String deviceId = null;
protected final int clientId;
IF userId is set
    VALIDATE userId (input)
ELSE IF uuid is set
    VALIDATE uuiduuId (input)
ELSE IF deviceId is set
    VALIDATE deviceId (input)
ELSE
    THROW IllegalStateException
IF userId not set AND uuiduuId not set AND deviceid not set
    THROW IllegalStateException
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 uuiduuId) {
    throwOnNullOrEmpty(uuid, "UUID");
    this.uuiduuId = uuid;uuId;
    return this;
}

public Builder setDeviceId(String deviceId) {
    throwOnNullOrEmpty(deviceId, "DEVICE ID");
    this.deviceId = deviceId;
    return this;
}
public DataKey build() {
    if(userId == null && uuiduuId == null && deviceId == null) {
        throw new IllegalStateException("You need to set at least one of userId, uuid or deviceId");
    }
    return new DataKey(this);
}
protected Long userid = null;
protected String uuid = null;
protected String deviceId = null;
protected final int clientId;
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
IF userId not set AND uuid not set AND deviceid not set
    THROW IllegalStateException
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;
}
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);
}
//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;
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
IF userId not set AND uuId not set AND deviceid not set
    THROW IllegalStateException
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;
}
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);
}
Source Link
Vogel612
  • 25.3k
  • 7
  • 58
  • 141

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

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