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

2 thoughts on “Java Constructor Anti-Pattern

  1. I feel as long as you keep the constructor argument name same as the field name. It should not happen. Also, if you use IDEs provides different color convention to fields vs function arguments.

  2. Yes, I agree that using a proper IDE usually either highlights this problem, or even auto-generates/-completes the code for you to avoid it.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s