patternjavaMinor
Acquiring a data key
Viewed 0 times
dataacquiringkey
Problem
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 c
```
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 c
Solution
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
Or even sooner:
then you could remove the validation "clutter" from your DataKey constructor. And your
If-Statement vs Ternary operator:
You can shortcut this drastically by using a ternary operator. IMO this also makes your intent way more clear:
You can shorten this even further by adding a static import to your class:
and then you get:
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
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;Code Snippets
private DataKey(Builder builder) {
if(!builder.isValid()){
throw new IllegalStateException(INVALID_BUILDER_MESSAGE);
}
//keep doing the stuff you do ;)
}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());
}if (userId != 0) {
this.flow = FlowEnum.USERFLOW;
}
else {
this.flow = FlowEnum.DEVICEFLOW;
}this.flow = (userId == 0) ? FlowEnum.DEVICEFLOW : FlowEnum.USERFLOW;Context
StackExchange Code Review Q#55585, answer score: 5
Revisions (0)
No revisions yet.