patternjavaMinor
Robust organization data class (partial followup)
Viewed 0 times
robustdatapartialfollowupclassorganization
Problem
This question is a partial followup from my previous question, however the requirements have changed: I now need to be able to store multiple data entries for some fields.
I also have added two hooks into the
And lastly, the requirement that all fields must be populated, has been dropped.
```
public class Organization implements Serializable {
private static final long serialVersionUID = 28484399283838343L;
private final EnumMap> multiFieldMap = new EnumMap<>(MultiOrganizationField.class);
private final EnumMap singleFieldMap = new EnumMap<>(SingleOrganizationField.class);
Organization(final EnumMap> multiFieldMap, final EnumMap singleFieldMap) {
this.multiFieldMap.putAll(multiFieldMap);
this.singleFieldMap.putAll(singleFieldMap);
}
public List getAccountNumbers() {
return Collections.unmodifiableList(multiFieldMap.get(ACCOUNT_NUMBER));
}
public Optional getAddress() {
return Optional.ofNullable(singleFieldMap.get(ADDRESS));
}
public Optional getBranchCode() {
return Optional.ofNullable(singleFieldMap.get(BRANCH_CODE));
}
public Optional getChamberOfCommerce() {
return Optional.ofNullable(singleFieldMap.get(CHAMBER_OF_COMMERCE));
}
public Optional getCity() {
retur
I also have added two hooks into the
OrganizationBuilder, one that validates the given input, and one that formats the given input, mainly used to achieve consistency.And lastly, the requirement that all fields must be populated, has been dropped.
public enum OrganizationBuilderMode {
THROW_EXCEPTION_ON_VALIDATION_FAILURE,
IGNORE_ON_VALIDATION_FAILURE;
}public interface OrganizationField { }public enum SingleOrganizationField implements OrganizationField {
ADDRESS,
BRANCH_CODE,
CHAMBER_OF_COMMERCE,
CITY,
ORGANIZATION_ID,
ORGANIZATION_NAME,
POSTAL_ADDRESS,
POSTAL_CITY,
POSTAL_ZIP_CODE,
VAT_NUMBER,
ZIP_CODE
}public enum MultiOrganizationField implements OrganizationField {
ACCOUNT_NUMBER,
IBAN_NUMBER,
}```
public class Organization implements Serializable {
private static final long serialVersionUID = 28484399283838343L;
private final EnumMap> multiFieldMap = new EnumMap<>(MultiOrganizationField.class);
private final EnumMap singleFieldMap = new EnumMap<>(SingleOrganizationField.class);
Organization(final EnumMap> multiFieldMap, final EnumMap singleFieldMap) {
this.multiFieldMap.putAll(multiFieldMap);
this.singleFieldMap.putAll(singleFieldMap);
}
public List getAccountNumbers() {
return Collections.unmodifiableList(multiFieldMap.get(ACCOUNT_NUMBER));
}
public Optional getAddress() {
return Optional.ofNullable(singleFieldMap.get(ADDRESS));
}
public Optional getBranchCode() {
return Optional.ofNullable(singleFieldMap.get(BRANCH_CODE));
}
public Optional getChamberOfCommerce() {
return Optional.ofNullable(singleFieldMap.get(CHAMBER_OF_COMMERCE));
}
public Optional getCity() {
retur
Solution
There are a number of things here to comment on. First up, you don't ask for anything in particular to be reviewed. So, I just scanned the code, looking for 'oddities'.
There are some aspects which concern me.
Marker interfaces are seldom a good idea. Why do you have
Additionally, you have a complciated SerializationProxy mechanism set up. I cannot see a reason to have it though. What's wrong with just plain serialization? If there is a reason, I can't see it, so it should probably be commented. Serialization proxies are an ugly workaround to an overly complicated process. Don't make it more complicated unless you really need them.
By all accounts, the whole purpose of these classes are to provide a way for a class to encapsulate a lot of data fields, and then allow those to be serialized. The data fields are stored in a Map. Classes of this type are unfortunately bulky, and ugly. Unfortunately, though, your solution is still bulky, and a different sort of ugly.
I would stick with the more traditional format of having a whole bunch of setter methods, and instance fields. Keep it only a standard form of ugly, not an indirected/abstracted ugly.
Finally, the use of Optionals is.... unconventional in this format. Perhaps things will change more with Java 8, but I can't see a reason why the methods have to return an optional, when a null will suffice. You are creating a lot of typically unnecessary overhead.
There are some aspects which concern me.
Marker interfaces are seldom a good idea. Why do you have
OrganizationField... I cannot see a single place where you actually use it as the marker. All actual usages are defined as either SingleOrganizationField or MultiOrganizationField. It adds no value that I can see.Additionally, you have a complciated SerializationProxy mechanism set up. I cannot see a reason to have it though. What's wrong with just plain serialization? If there is a reason, I can't see it, so it should probably be commented. Serialization proxies are an ugly workaround to an overly complicated process. Don't make it more complicated unless you really need them.
By all accounts, the whole purpose of these classes are to provide a way for a class to encapsulate a lot of data fields, and then allow those to be serialized. The data fields are stored in a Map. Classes of this type are unfortunately bulky, and ugly. Unfortunately, though, your solution is still bulky, and a different sort of ugly.
I would stick with the more traditional format of having a whole bunch of setter methods, and instance fields. Keep it only a standard form of ugly, not an indirected/abstracted ugly.
Finally, the use of Optionals is.... unconventional in this format. Perhaps things will change more with Java 8, but I can't see a reason why the methods have to return an optional, when a null will suffice. You are creating a lot of typically unnecessary overhead.
Context
StackExchange Code Review Q#57031, answer score: 2
Revisions (0)
No revisions yet.