Split condition

Intuitively it makes sense to group several statements in an if/else clause. In that case you only need to execute the check once. However, this is not always the cleanest solution, and it makes methods longer than they should be.

For example, take the following piece of code.

    public void updateDocument(Attachment attachment){
        Document document = new Document();
        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            DocumentType documentType = repository.findByCode(DocumentType.class, DocumentType.CODE_SPECIAL);
            document.setType(documentType);
            document.setName(documentType.getName() + " " + getDateAsString());
        } else {
            document.setType(repository.findByCode(DocumentType.class, DocumentType.CODE_OTHER));
            document.setName("Attachment " + getDateAsString());
        }
    }

So, what’s wrong with this? It does what it’s supposed to do, and it doesn’t look too complicated.

But it does too many things:

  1. It checks the type of the attachment
  2. Based on this type, it finds the DocumentType from the database
  3. It sets the DocumentType of the Document
  4. It generates a name
  5. It sets the name of the document

To cite Uncle Bob (Clean Code, page 35):

Functions should do one thing. They should do it well. They should do it only.

So, let’s refactor.

First, copy the condition, and move the statements around so that each condition handles only one property of the document.

    public void updateDocument(Attachment attachment){
        Document document = new Document();
        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            DocumentType documentType = repository.findByCode(DocumentType.class, DocumentType.CODE_SPECIAL);
            document.setType(documentType);
        } else {
            document.setType(repository.findByCode(DocumentType.class, DocumentType.CODE_OTHER));
        }

        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())){
            document.setName(documentType.getName() + " " + getDateAsString());
        } else {
            document.setName("Attachment " + getDateAsString());
        }
    }

This doesn’t compile, because documentType isn’t available in the second block. So, let’s move the declaration of DocumentType outside of the first block.

    public void updateDocument(Attachment attachment){
        Document document = new Document();

        DocumentType documentType = null;
        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            documentType = repository.findByCode(DocumentType.class, DocumentType.CODE_SPECIAL);
            document.setType(documentType);
        } else {
            document.setType(repository.findByCode(DocumentType.class, DocumentType.CODE_OTHER));
        }

        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            document.setName(documentType.getName() + " " + getDateAsString());
        } else {
            document.setName("Attachment " + getDateAsString());
        }
    }

Ah, that’s better. But there’s duplication: document.setType is called on two lines. Let’s move that outside of the if block. And, while we’re at it, let’s do the same for setName.

    public void updateDocument(Attachment attachment){
        Document document = new Document();

        DocumentType documentType = null;
        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            documentType = repository.findByCode(DocumentType.class, DocumentType.CODE_SPECIAL);
        } else {
            documentType = repository.findByCode(DocumentType.class, DocumentType.CODE_OTHER)
        }
        document.setType(documentType);

        String name = null;
        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            name = documentType.getName() + " " + getDateAsString();
        } else {
            name = "Attachment " + getDateAsString();
        }
        document.setName(name);
    }

This is getting in better shape. Now we have separate logic for getting the DocumentType, setting the DocumentType, getting the name, and setting the name. Let’s extract some methods and see what happens.

    public void updateDocument(Attachment attachment){
        Document document = new Document();
        DocumentType documentType = getDocumentType(attachment);
        document.setType(documentType);

        String name = getName(attachment, documentType);
        document.setName(name);
    }

    private DocumentType getDocumentType(Attachment attachment) {
        DocumentType documentType = null;
        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            documentType = repository.findByCode(DocumentType.class, DocumentType.CODE_SPECIAL);
        } else {
            documentType = repository.findByCode(DocumentType.class, DocumentType.CODE_OTHER)
        }
        return documentType;
    }

    private String getName(Attachment attachment, DocumentType documentType) {
        String name = null;
        if (AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType())) {
            name = documentType.getName() + " " + getDateAsString();
        } else {
            name = "Attachment " + getDateAsString();
        }
        return name;
    }

Our original method looks a lot simpler now. But the methods we’ve extraced contain duplication! The condition itself, the calls to repository.findByCode, and the calls to getDateAsString(). So, let’s fix that.

    public void updateDocument(Attachment attachment){
        Document document = new Document();

        DocumentType documentType = getDocumentType(attachment);
        document.setType(documentType);

        String name = getName(attachment, documentType);
        document.setName(name);
    }

    private DocumentType getDocumentType(Attachment attachment) {
        String documentTypeCode = null; 
        if (isAttachmentTypeSpecial(attachment)) {
            documentTypeCode = DocumentType.CODE_SPECIAL;
        } else {
            documentTypeCode = DocumentType.CODE_OTHER;
        }
        return repository.findByCode(DocumentType.class, documentTypeCode);
    }

    private String getName(Attachment attachment, DocumentType documentType) {
        String name = null;
        if (isAttachmentTypeSpecial(attachment)) {
            name = documentType.getName();
        } else {
            name = "Attachment";
        }
        return name + " " + getDateAsString();
    }

    private boolean isAttachmentTypeSpecial(Attachment attachment) {
        return AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType());
    }

This code still looks a bit odd. getDocumentType() determines the document type code, and then uses that to find the documentType. And getName() has a similar issue: it determines the prefix for the name, and then appends the date as String.

    public void updateDocument(Attachment attachment){
        Document document = new Document();

        DocumentType documentType = getDocumentType(attachment);
        document.setType(documentType);

        String name = getName(attachment, documentType);
        document.setName(name);
    }

    private DocumentType getDocumentType(Attachment attachment) {
        String documentTypeCode = getDocumentTypeCode(attachment);
        return repository.findByCode(DocumentType.class, documentTypeCode);
    }

    private String getDocumentTypeCode(Attachment attachment) {
        if (isAttachmentTypeSpecial(attachment)) {
            return DocumentType.CODE_SPECIAL;
        } 
        return DocumentType.CODE_OTHER;
    }

    private String getName(Attachment attachment, DocumentType documentType) {
        String name = getNamePrefix(attachment, documentType);
        return name + " " + getDateAsString();
    }

    private String getNamePrefix(Attachment attachment, DocumentType documentType) {
        if (isAttachmentTypeSpecial(attachment)) {
            return documentType.getName();
        } 
        return "Attachment";
    }

    private boolean isAttachmentTypeSpecial(Attachment attachment) {
        return AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType());
    }

At this point we have some variables and perhaps some methods that are not adding anything in terms of clarity. We should inline them and compact our code a little.

    public void updateDocument(Attachment attachment){
        Document document = new Document();

        DocumentType documentType = getDocumentType(attachment);
        document.setType(documentType);

        document.setName(getName(attachment, documentType));
    }

    private DocumentType getDocumentType(Attachment attachment) {
        return repository.findByCode(DocumentType.class, getDocumentTypeCode(attachment));
    }

    private String getDocumentTypeCode(Attachment attachment) {
        if (isAttachmentTypeSpecial(attachment)) {
            return DocumentType.CODE_SPECIAL;
        }
        return DocumentType.CODE_OTHER;
    }

    private String getName(Attachment attachment, DocumentType documentType) {
        return getNamePrefix(attachment, documentType) + " " + getDateAsString();
    }

    private String getNamePrefix(Attachment attachment, DocumentType documentType) {
        if (isAttachmentTypeSpecial(attachment)) {
            return documentType.getName();
        }
        return "Attachment";
    }

    private boolean isAttachmentTypeSpecial(Attachment attachment) {
        return AttachmentModel.TYPE_SPECIAL.equals(attachment.getAttachmentType());
    }

This refactoring introduced a couple of new methods. Each method call takes a little extra time, and reduces performance a bit. But there’s a catch: with modern processors, cache-misses are the biggest performance killers. Keeping the data as local as possible will reduce cache-misses and increase performance.
In practice, however, I doubt that you’ll even notice the performance difference.

As for clarity, the original code expanded from 11 lines to 34 lines, a 3-fold increase. However, the original method decreased a bit. And because we’ve added a couple of layers of abstraction, the method now communicates only what it does. The details of how are delegated to other methods.

Leave a Reply

Your email address will not be published. Required fields are marked *