4 of 9
added 239 characters in body

Not code, but refactoring: reduce cognitive complexity by extracting private methods

This is code, written by our dev team, and SonarQube tells me that the Cognitive Complexity is too high: 21, and the current (default, out of the box) metric limit is 15. I've had a go at reducing the complexity and would love some feedback.

For those interested, the full source code is also available on GitHub.

The code does the following:

  • Is the current PDF document instance encrypted -> bail out
  • Is the current PDF unencrypted, get the embedded file inside the wrapper document, if any
  • Return the encrypted payload that is inside the wrapper document

For more details, please refer to the PDF specification ISO 32000-1:2008.


Original code

/**
 * Gets the encrypted payload of this document,
 * or returns {@code null} if this document isn't an unencrypted wrapper document.
 *
 * @return encrypted payload of this document.
 */
public PdfEncryptedPayloadDocument getEncryptedPayloadDocument2() {
    if (getReader() != null && getReader().isEncrypted()) {
        return null;
    }
    PdfCollection collection = getCatalog().getCollection();
    if (collection != null && collection.isViewHidden()) {
        PdfString documentName = collection.getInitialDocument();
        PdfNameTree embeddedFiles = getCatalog().getNameTree(PdfName.EmbeddedFiles);
        String documentNameUnicode = documentName.toUnicodeString();
        PdfObject fileSpecObject = embeddedFiles.getNames().get(documentNameUnicode);
        if (fileSpecObject != null && fileSpecObject.isDictionary()) {
            try {
                PdfFileSpec fileSpec = PdfEncryptedPayloadFileSpecFactory.wrap((PdfDictionary) fileSpecObject);
                if (fileSpec != null) {
                    PdfDictionary embeddedDictionary = ((PdfDictionary) fileSpec.getPdfObject()).getAsDictionary(PdfName.EF);
                    PdfStream stream = embeddedDictionary.getAsStream(PdfName.UF);
                    if (stream == null) {
                        stream = embeddedDictionary.getAsStream(PdfName.F);
                    }
                    if (stream != null) {
                        return new PdfEncryptedPayloadDocument(stream, fileSpec, documentNameUnicode);
                    }
                }
            } catch (PdfException e) {
                LoggerFactory.getLogger(getClass()).error(e.getMessage());
            }
        }
    }
    return null;
}

Goals of the refactoring:

  • lower complexity
  • better readability
  • better testability
  • better maintainability

Refactored code

/**
 * Gets the encrypted payload of this document,
 * or returns {@code null} if this document isn't an unencrypted wrapper document.
 *
 * @return encrypted payload of this document.
 */
public PdfEncryptedPayloadDocument getEncryptedPayloadDocument2() {
    if (readerIsEncrypted(getReader())) {
        return null;
    }
    PdfCollection collection = getCatalog().getCollection();
    if (collectionIsNotViewHidden(collection)) {
        return null;
    }
    PdfString documentName = collection.getInitialDocument();
    PdfNameTree embeddedFiles = getCatalog().getNameTree(PdfName.EmbeddedFiles);
    String documentNameUnicode = documentName.toUnicodeString();
    PdfObject fileSpecObject = embeddedFiles.getNames().get(documentNameUnicode);
    if (fileSpecObjectIsNoDictionary(fileSpecObject)) {
        return null;
    }
    try {
        PdfFileSpec fileSpec = PdfEncryptedPayloadFileSpecFactory.wrap((PdfDictionary) fileSpecObject);
        if (fileSpec != null) {
            PdfDictionary embeddedDictionary = ((PdfDictionary) fileSpec.getPdfObject()).getAsDictionary(PdfName.EF);
            PdfStream stream = getPdfStreamUf(embeddedDictionary);
            if (stream != null) {
                return new PdfEncryptedPayloadDocument(stream, fileSpec, documentNameUnicode);
            }
        }
    } catch (PdfException e) {
        LoggerFactory.getLogger(getClass()).error(e.getMessage());
    }
    return null;
}

private boolean readerIsEncrypted(PdfReader reader) {
    return reader != null && reader.isEncrypted();
}

private boolean collectionIsNotViewHidden(PdfCollection collection) {
    return collection == null || !collection.isViewHidden();
}

private boolean fileSpecObjectIsNoDictionary(PdfObject fileSpecObject) {
    return fileSpecObject == null || !fileSpecObject.isDictionary();
}

private PdfStream getPdfStreamUf(PdfDictionary dictionary) {
    PdfStream stream = dictionary.getAsStream(PdfName.UF);
    if (stream == null) {
        stream = dictionary.getAsStream(PdfName.F);
    }
    return stream;
}

git diff

@ -1557,36 +1513,55 @@ public class PdfDocument implements IEventDispatcher, Closeable, Serializable {
      * @return encrypted payload of this document.
      */
     public PdfEncryptedPayloadDocument getEncryptedPayloadDocument2() {
-        if (getReader() != null && getReader().isEncrypted()) {
+        if (readerIsEncrypted(getReader())) {
             return null;
         }
         PdfCollection collection = getCatalog().getCollection();
-        if (collection != null && collection.isViewHidden()) {
-            PdfString documentName = collection.getInitialDocument();
-            PdfNameTree embeddedFiles = getCatalog().getNameTree(PdfName.EmbeddedFiles);
-            String documentNameUnicode = documentName.toUnicodeString();
-            PdfObject fileSpecObject = embeddedFiles.getNames().get(documentNameUnicode);
-            if (fileSpecObject != null && fileSpecObject.isDictionary()) {
-                try {
-                    PdfFileSpec fileSpec = PdfEncryptedPayloadFileSpecFactory.wrap((PdfDictionary) fileSpecObject);
-                    if (fileSpec != null) {
-                        PdfDictionary embeddedDictionary = ((PdfDictionary) fileSpec.getPdfObject()).getAsDictionary(PdfName.EF);
-                        PdfStream stream = embeddedDictionary.getAsStream(PdfName.UF);
-                        if (stream == null) {
-                            stream = embeddedDictionary.getAsStream(PdfName.F);
-                        }
-                        if (stream != null) {
-                            return new PdfEncryptedPayloadDocument(stream, fileSpec, documentNameUnicode);
-                        }
-                    }
-                } catch (PdfException e) {
-                    LoggerFactory.getLogger(getClass()).error(e.getMessage());
+        if (collectionIsNotViewHidden(collection)) {
+            return null;
+        }
+        PdfString documentName = collection.getInitialDocument();
+        PdfNameTree embeddedFiles = getCatalog().getNameTree(PdfName.EmbeddedFiles);
+        String documentNameUnicode = documentName.toUnicodeString();
+        PdfObject fileSpecObject = embeddedFiles.getNames().get(documentNameUnicode);
+        if (fileSpecObjectIsNoDictionary(fileSpecObject)) {
+            return null;
+        }
+        try {
+            PdfFileSpec fileSpec = PdfEncryptedPayloadFileSpecFactory.wrap((PdfDictionary) fileSpecObject);
+            if (fileSpec != null) {
+                PdfDictionary embeddedDictionary = ((PdfDictionary) fileSpec.getPdfObject()).getAsDictionary(PdfName.EF);
+                PdfStream stream = getPdfStreamUf(embeddedDictionary);
+                if (stream != null) {
+                    return new PdfEncryptedPayloadDocument(stream, fileSpec, documentNameUnicode);
                 }
             }
+        } catch (PdfException e) {
+            LoggerFactory.getLogger(getClass()).error(e.getMessage());
         }
         return null;
     }
 
+    private boolean readerIsEncrypted(PdfReader reader) {
+        return reader != null && reader.isEncrypted();
+    }
+
+    private boolean collectionIsNotViewHidden(PdfCollection collection) {
+        return collection == null || !collection.isViewHidden();
+    }
+
+    private boolean fileSpecObjectIsNoDictionary(PdfObject fileSpecObject) {
+        return fileSpecObject == null || !fileSpecObject.isDictionary();
+    }
+
+    private PdfStream getPdfStreamUf(PdfDictionary dictionary) {
+        PdfStream stream = dictionary.getAsStream(PdfName.UF);
+        if (stream == null) {
+            stream = dictionary.getAsStream(PdfName.F);
+        }
+        return stream;
+    }
+
     /**
      * Sets an encrypted payload, making this document an unencrypted wrapper document.
      * The file spec shall include the AFRelationship key with a value of EncryptedPayload,

Summary of changes:

  • invert the second if, to have the bottom return null earlier.

  • invert the third if, also to have a return null earlier.

    These two changes add one exit point to the method, and are used as guard clauses, something I found on Martin Fowler's website refactoring.com.

  • extract to a private method: getting the dictionary as a stream and checking if it is null and then getting it in another way.

  • extract all the compound conditions to private methods.


The refactored code now has a cognitive complexity of 7, well below the alert level of 15, which means that at least one of the stated goals was achieved.

What I, from the standpoint as a tester, see as a bonus on my refactored code, is that I can look at the extracted private methods in my IDE, see that they are only partially covered by unit tests (for example 2 out of 4 conditions), and based on that information (white box testing), design a new unit test that accesses the public API to cover the missing conditions.

I am also considering to make the private methods static, because they only act on the input and don't use anything from the class instance. But I'm neutral on that point, it could go either way.

The concern of the developer that reported the high Cognitive Complexity, is that splitting the method up in smaller methods, would actually reduce readability of the code.

Questions

  • Please do not review the original code. It works, it does exactly what it is intended to do.
  • Please do review the refactoring, that is, the diff between the old code and the new code. As far as testing can tell me, functionality should be 100% identical.
  • Please do give suggestions on how the refactoring can be further improved. One of the items that pops into mind, is better names for the extracted methods.

About me

I'm a Stack Exchange veteran (see profile), but this is my first question on the Code Review site. I'm a tester, not a developer, and it's been over a decade since I last wrote any Java code. So please be gentle with me. :-)