Java Constructor Anti-Pattern

I have never liked the typical java way of creating constructors for a typical “bean” class. The preferred way, by many, is to have the constructor arguments directly map to the fields in the class, and assign each in turn to the “this.” prefixed name.

I have always preferred to have the constructor arguments be short one-or-two-letter variables that you assign to the fields. This avoids the following problem I found today:

public class MetadataEntityDescriptor {
    private ContactPerson contactPerson;
    private IDPSSODescriptor idpSsoDescriptor;

    private MetadataEntityDescriptor(IDPSSODescriptor idpssoDescriptor,
                                     ContactPerson contactPerson) {
        this.idpSsoDescriptor = idpSsoDescriptor;
        this.contactPerson = contactPerson;
    }

    public static final class Builder {
        private IDPSSODescriptor idpssoDescriptor;
        private ContactPerson contactPerson;

        public Builder setIdpssoDescriptor(IDPSSODescriptor idpssoDescriptor) {
            this.idpssoDescriptor = idpssoDescriptor;
            return this;
        }

        public Builder setContactPerson(ContactPerson contactPerson) {
            this.contactPerson = contactPerson;
            return this;
        }

        public MetadataEntityDescriptor build() {
            return new MetadataEntityDescriptor(idpssoDescriptor, contactPerson);
        }
    }

    public Element asXml() {
        return new Element("EntityDescriptor", Constants.NAMESPACE_METADATA)
            .addContent(idpSsoDescriptor.asXml())
            .addContent(contactPerson.asXml());
    }
}

Why does the asXml() method here give a NullPointerException? The code looks good and even compiles. But notice the subtle capitalization bug in the first parameter of the constructor… In fact, the argument was never assigned to the field, and it thus stayed “null”. Fortunately my IDE caught it, but I did not see that until I had scratched my head a few times regarding the strange NPE.

Now, I will admit that an argument FOR having complete argument names is that it looks nicer in the IDE when you see the argument popup help. But to me that is not enough; I’d rather have shorter and less error prone code such as this:

public class MetadataEntityDescriptor {
    private ContactPerson contactPerson;
    private IDPSSODescriptor idpSsoDescriptor;

    private MetadataEntityDescriptor(IDPSSODescriptor isd,
                                     ContactPerson cp) {
        idpSsoDescriptor = isd;
        contactPerson = cp;
    }

    ...
}

And finally, this is of course a great argument for moving to Scala, where the equivalent, error proof code would be:

class MetadataEntityDescriptor(var contactPerson: ContactPerson,
                               var idpSsoDescriptor: IDPSSODescriptor) {
  def asXml = <EntityDescriptor>
      { idpSsoDescriptor.asXml }
      { contactPerson.asXml }
  </EntityDescriptor>
}
Advertisements