I recently was asked by a colleague how to get rid of a staggering amount of if statements.
They all seem to have remarkable similarities, as is obvious from the code below.
public List<ValidationMessage> verifyAddress(Address address) | |
{ | |
List<ValidationMessage> messages = new ArrayList<>(); | |
if (address.getHousenumber() == null) | |
{ | |
messages.add(new ValidationMessage(AddressPart.HOUSENUMBER, "Housenumber is missing")); | |
} | |
if (address.getHousenumber() != null && (address.getHousenumber() < 0 || address.getHousenumber() > 50000)) | |
{ | |
messages.add(new ValidationMessage(AddressPart.HOUSENUMBER, "Housenumber should be between 0 and 50000")); | |
} | |
if (address.getCity() == null) | |
{ | |
messages.add(new ValidationMessage(AddressPart.CITY, "City is missing")); | |
} | |
if (address.getStreetName() == null) | |
{ | |
messages.add(new ValidationMessage(AddressPart.STREETNAME, "Streetname is missing")); | |
} | |
if (address.getState() == null) | |
{ | |
messages.add(new ValidationMessage(AddressPart.STATE, "State is missing")); | |
} else if (address.getState().length() != 2) | |
{ | |
messages.add(new ValidationMessage(AddressPart.STATE, "Expecting State to be a 2-letter abbreviation")); | |
} | |
if (address.getSuffix() != null && address.getSuffix().length() != 1) | |
{ | |
messages.add(new ValidationMessage(AddressPart.SUFFIX, "Suffix to a house number should be just one letter")); | |
} | |
return messages; | |
} |
So I initially thought about changing it to:
public List<ValidationMessage> verifyAddress3(Address address) | |
{ | |
Map<Boolean, ValidationMessage> map = new HashMap<>(); | |
map.put(address.getHousenumber() == null, new ValidationMessage(AddressPart.HOUSENUMBER, "Housenumber is missing")); | |
map.put(address.getHousenumber() != null && (address.getHousenumber() < 0 || address.getHousenumber() > 50000), | |
new ValidationMessage(AddressPart.HOUSENUMBER, "Housenumber should be between 0 and 50000")); | |
map.put(address.getCity() == null, new ValidationMessage(AddressPart.CITY, "City is missing")); | |
map.put(address.getStreetName() == null, new ValidationMessage(AddressPart.STREETNAME, "Streetname is missing")); | |
map.put(address.getState() == null, new ValidationMessage(AddressPart.STATE, "State is missing")); | |
map.put(address.getState() != null && address.getState().length() != 2, new ValidationMessage(AddressPart.STATE, "Expecting State to be a 2-letter abbreviation")); | |
map.put(address.getSuffix() != null && address.getSuffix().length() != 1, new ValidationMessage(AddressPart.SUFFIX, "Suffix to a house number should be just one letter")); | |
return map.entrySet().stream() | |
.filter(Map.Entry::getKey) | |
.map(Map.Entry::getValue) | |
.collect(Collectors.toList()); | |
} |
Of course it is also possible to create a static Map, that contains Functions. That way, the ValidationMessage instances are only created once. It depends on your needs, really.
private static final Map<Function<Address, Boolean>, ValidationMessage> map = new HashMap<>(); | |
static | |
{ | |
map.put(address -> address.getHousenumber() == null, new ValidationMessage(AddressPart.HOUSENUMBER, "Housenumber is missing")); | |
map.put(address -> address.getHousenumber() != null && (address.getHousenumber() < 0 || address.getHousenumber() > 50000), | |
new ValidationMessage(AddressPart.HOUSENUMBER, "Housenumber should be between 0 and 50000")); | |
map.put(address -> address.getCity() == null, new ValidationMessage(AddressPart.CITY, "City is missing")); | |
map.put(address -> address.getStreetName() == null, new ValidationMessage(AddressPart.STREETNAME, "Streetname is missing")); | |
map.put(address -> address.getState() == null, new ValidationMessage(AddressPart.STATE, "State is missing")); | |
map.put(address -> address.getState() != null && address.getState().length() != 2, new ValidationMessage(AddressPart.STATE, "Expecting State to be a 2-letter abbreviation")); | |
map.put(address -> address.getSuffix() != null && address.getSuffix().length() != 1, new ValidationMessage(AddressPart.SUFFIX, "Suffix to a house number should be just one letter")); | |
} | |
public List<ValidationMessage> verifyAddress(Address address) | |
{ | |
return map.entrySet().stream() | |
.filter((x) -> x.getKey().apply(address)) | |
.map(Map.Entry::getValue) | |
.collect(Collectors.toList()); | |
} |
In the end, I don't know if the improvement is really that huge.
It's closer together at least, but I feel it could be improved.