Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I believe I have made the following data class quite decently, and I'd like a thorough review on it.

The code is built on Java 8 and uses the Builder and Serialization Proxy pattern, it is used to store results from a web service call, and the only certainty I have is that the organization id will be an integer, as it's most likely used as a primary key in some database.

public class Organization implements Serializable {
    private static final long serialVersionUID = 28484399283838343L;

    private final int organizationId;

    private final String accountNumber;
    private final String address;
    private final String branchCode;
    private final String chamberOfCommerce;
    private final String city;
    private final String ibanNumber;
    private final String organizationName;
    private final String postalAddress;
    private final String postalCity;
    private final String postalZipCode;
    private final String vatNumber;
    private final String zipCode;

    Organization(final int organizationId, final String accountNumber, final String address, final String branchCode, final String chamberOfCommerce, final String city, final String ibanNumber, final String organizationName, final String postalAddress, final String postalCity, final String postalZipCode, final String vatNumber, final String zipCode) {
        this.organizationId = organizationId;
        this.accountNumber = Objects.requireNonNull(accountNumber, "accountNumber");
        this.address = Objects.requireNonNull(address, "address");
        this.branchCode = Objects.requireNonNull(branchCode, "branchCode");
        this.chamberOfCommerce = Objects.requireNonNull(chamberOfCommerce, "chamberOfCommerce");
        this.city = Objects.requireNonNull(city, "city");
        this.ibanNumber = Objects.requireNonNull(ibanNumber, "ibanNumber");
        this.organizationName = Objects.requireNonNull(organizationName, "organizationName");
        this.postalAddress = Objects.requireNonNull(postalAddress, "postalAddress");
        this.postalCity = Objects.requireNonNull(postalCity, "postalCity");
        this.postalZipCode = Objects.requireNonNull(postalZipCode, "postalZipCode");
        this.vatNumber = Objects.requireNonNull(vatNumber, "vatNumber");
        this.zipCode = Objects.requireNonNull(zipCode, "zipCode");
    }

    public int getOrganizationId() {
        return organizationId;
    }

    public String getAccountNumber() {
        return accountNumber;
    }

    public String getAddress() {
        return address;
    }

    public String getBranchCode() {
        return branchCode;
    }

    public String getChamberOfCommerce() {
        return chamberOfCommerce;
    }

    public String getCity() {
        return city;
    }

    public String getIbanNumber() {
        return ibanNumber;
    }

    public String getOrganizationName() {
        return organizationName;
    }

    public String getPostalAddress() {
        return postalAddress;
    }

    public String getPostalCity() {
        return postalCity;
    }

    public String getPostalZipCode() {
        return postalZipCode;
    }

    public String getVatNumber() {
        return vatNumber;
    }

    public String getZipCode() {
        return zipCode;
    }

    private Object writeReplace() {
        return new SerializationProxy(this);
    }

    private void readObject(final ObjectInputStream stream) throws InvalidObjectException {
        throw new InvalidObjectException("Proxy required");
    }

    private static class SerializationProxy implements Serializable {
        private static final long serialVersionUID = 484894399233454L;

        private final int organizationId;

        private final String accountNumber;
        private final String address;
        private final String branchCode;
        private final String chamberOfCommerce;
        private final String city;
        private final String ibanNumber;
        private final String organizationName;
        private final String postalAddress;
        private final String postalCity;
        private final String postalZipCode;
        private final String vatNumber;
        private final String zipCode;

        private SerializationProxy(final Organization organization) {
            this.organizationId = organization.organizationId;
            this.accountNumber = organization.accountNumber;
            this.address = organization.address;
            this.branchCode = organization.branchCode;
            this.chamberOfCommerce = organization.chamberOfCommerce;
            this.city = organization.city;
            this.ibanNumber = organization.ibanNumber;
            this.organizationName = organization.organizationName;
            this.postalAddress = organization.postalAddress;
            this.postalCity = organization.postalCity;
            this.postalZipCode = organization.postalZipCode;
            this.vatNumber = organization.vatNumber;
            this.zipCode = organization.zipCode;
        }

        private Object readResolve() {
            return new Organization(
                    organizationId, accountNumber, address, branchCode, 
                    chamberOfCommerce, city, ibanNumber, organizationName, 
                    postalAddress, postalCity, postalZipCode, vatNumber, 
                    zipCode
            );
        }
    }
}

public class OrganizationBuilder {
    private OptionalInt organizationId = OptionalInt.empty();

    private Optional<String> accountNumber = Optional.empty();
    private Optional<String> address = Optional.empty();
    private Optional<String> branchCode = Optional.empty();
    private Optional<String> chamberOfCommerce = Optional.empty();
    private Optional<String> city = Optional.empty();
    private Optional<String> ibanNumber = Optional.empty();
    private Optional<String> organizationName = Optional.empty();
    private Optional<String> postalAddress = Optional.empty();
    private Optional<String> postalCity = Optional.empty();
    private Optional<String> postalZipCode = Optional.empty();
    private Optional<String> vatNumber = Optional.empty();
    private Optional<String> zipCode = Optional.empty();

    public OrganizationBuilder organizationId(final int organizationId) {
        this.organizationId = OptionalInt.of(organizationId);
        return this;
    }

    public OrganizationBuilder accountNumber(final String accountNumber) {
        this.accountNumber = Optional.of(accountNumber);
        return this;
    }

    public OrganizationBuilder address(final String address) {
        this.address = Optional.of(address);
        return this;
    }

    public OrganizationBuilder branchCode(final String branchCode) {
        this.branchCode = Optional.of(branchCode);
        return this;
    }

    public OrganizationBuilder chamberOfCommerce(final String chamberOfCommerce) {
        this.chamberOfCommerce = Optional.of(chamberOfCommerce);
        return this;
    }

    public OrganizationBuilder city(final String city) {
        this.city = Optional.of(city);
        return this;
    }

    public OrganizationBuilder ibanNumber(final String ibanNumber) {
        this.ibanNumber = Optional.of(ibanNumber);
        return this;
    }

    public OrganizationBuilder organizationName(final String organizationName) {
        this.organizationName = Optional.of(organizationName);
        return this;
    }

    public OrganizationBuilder postalAddress(final String postalAddress) {
        this.postalAddress = Optional.of(postalAddress);
        return this;
    }

    public OrganizationBuilder postalCity(final String postalCity) {
        this.postalCity = Optional.of(postalCity);
        return this;
    }

    public OrganizationBuilder postalZipCode(final String postalZipCode) {
        this.postalZipCode = Optional.of(postalZipCode);
        return this;
    }

    public OrganizationBuilder vatNumber(final String vatNumber) {
        this.vatNumber = Optional.of(vatNumber);
        return this;
    }

    public OrganizationBuilder zipCode(final String zipCode) {
        this.zipCode = Optional.of(zipCode);
        return this;
    }

    public Organization build() {
        if (!organizationId.isPresent() || !accountNumber.isPresent() || !address.isPresent() || 
                !branchCode.isPresent() || !chamberOfCommerce.isPresent() || !city.isPresent() ||
                !ibanNumber.isPresent() || !organizationName.isPresent() || !postalAddress.isPresent() ||
                !postalCity.isPresent() || !postalZipCode.isPresent() || !vatNumber.isPresent() ||
                !zipCode.isPresent()) {
            throw new IllegalStateException("Not all elements have been specified");
        }
        return new Organization(
                organizationId.getAsInt(), accountNumber.get(), address.get(), branchCode.get(),
                chamberOfCommerce.get(), city.get(), ibanNumber.get(), organizationName.get(),
                postalAddress.get(), postalCity.get(), postalZipCode.get(), vatNumber.get(),
                zipCode.get()
        );
    }
}

Small hint: What happens if the web service gets updated and returns additional fields?

share|improve this question

1 Answer 1

up vote 10 down vote accepted

This code is begging for an EnumMap as the data store, rather than the individual variables.

Creating an Enum with values like:

public enum OrganizationField {ACCOUNT_NUMBER, ZIP_CODE, ....};

In your Organization you would have:

private final EnumMap<OrganizationField,String> fieldMap = new ...;

and methods like:

public String getAccountNumber() {
    return fieldMap.get(OrganizationField.ACCOUNT_NUMBER);
}

Then, your class can simply have an EnumMap and the organizationId as fields. The serialization should serialize the map, and then, if the fields change, the serialization does not need to change. All you have to do is add a new Enum member, and a new 'getter' that pulls that Enum's value from the map.

Similarly, the OrganizationBuilder can simply loop through the Enum to make sure all members are populated in the map before construction:

for (OrganizationField f : OrganizationField.values()) {
    if (!fieldMap.containsKey(f)) {
        throw new IllegalStateException("Field " + f + " has not been specified");
    }
}

which gives the added bonus of knowing which field was not populated.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.