Get the encrypted payload from an unencrypted wrapper PDF document

Question

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

• The original code is intended as documentation, and comes from an Open Source repository on GitHub (link provided at start of question).
• Please 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. My personal goal is to become better at refactoring legacy software.
• 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. :-)

Answer 1

Positive Changes

• getReader() is only called once, while called multiple times before.
• There are more early exits as before, which results in less nested and lesser deepened nested statements.
• Method getPdfStreamUf hides the fallback method away from the main method, this functionality deserves its own method.

Negative Changes

• I agree with the developer that methods as readerIsEncrypted, collectionIsNotViewHidden and fileSpecObjectIsNoDictionary reduce readability. These methods are just glorified wrappers for other methods with a null check included.
• You still keep some conditions in that could have exited early if (fileSpec != null) and if (stream != null); you could reduce nested statements further if you'd return null inverted here also.

Other Observations

• getCatalog() is still called multiple times. Call it once and cache it in a local variable.
• Make variables that don't change after being instantiated final.
• All in all I would prefer readability and consistency over some complexity metric, although this metric could be an indication to refactor the code. If you split up methods, make sure each method adheres to the Single Responsibility Principle (though this could be interpreted any way you want :)

Answer 2

PDF specification ISO 32000-1:2008.

Funny, that's a roughly 800 page document if I see that right :) You might want to link to relevant parts if you want a reader to read it, otherwise that doesn't really help with the review here.

The refactored version has less nesting, yes, so that's somewhat easier to read. However I've the feeling that the extracted helpers are bit too short, essentially they're wrapping the null check and then combine that with another attribute; it's not that much shorter after all. (Edit: Actually maybe they just need better names. Best if there's no negation in the names, notHidden ... well, that's just visible, isn't it?)

I was also going to make the comment about caching getters. But. Clear code is more important and one additional line for x = getX() looks super pointless when x is used only twice (or maybe even three times). That's all assuming that getters simple return fields and do not have more hidden logic. Otherwise caching might of course be necessary for performance reasons.

In any case the denesting could go even further, after all there are two more if's of the same nature:

    ...
    try {
        PdfFileSpec fileSpec = PdfEncryptedPayloadFileSpecFactory.wrap((PdfDictionary) fileSpecObject);
        if (fileSpec == null) {
            return null;
        }
        PdfDictionary embeddedDictionary = ((PdfDictionary) fileSpec.getPdfObject()).getAsDictionary(PdfName.EF);
        PdfStream stream = getPdfStreamUf(embeddedDictionary);
        if (stream == null) {
            return null;
        }
        return new PdfEncryptedPayloadDocument(stream, fileSpec, documentNameUnicode);
    } catch (PdfException e) {
        logger.error(e);
        return null;
    }
}

Btw. is the try around the smallest section of the code that can throw? Otherwise It'd help to move it closer to the section that can actually fail, possibly allowing some more restructuring too.

Unfortunately all the null checks can't easily be refactored in Java like it could be done in e.g. Scala (probably Kotlin). That also limits how this code can be structured as it's a very imperative sequence.

I'd otherwise try and create more meaningful helpers: If that's at all possible I'd consider something like getDocumentName:

private String getDocumentName() {
    return getCatalog().getCollection().getInitialDocument().toUnicodeString();
}

Clear meaning to what this does (assuming the Unicode bit is less important here, otherwise the name would've to encode that).

The casts are always a bit ugly, how about:

private PdfNameTree getEmbeddedFiles() {
    return getCatalog().getNameTree(PdfName.EmbeddedFiles);
}

private PdfDictionary getFileSpecDictionary(PdfNameTree embeddedFiles) {
    PdfObject fileSpecObject = embeddedFiles.getNames().get(getDocumentName());
    if (fileSpecObject == null || !fileSpecObject.isDictionary()) {
        return nil;
    }
    return (PdfDictionary) fileSpecObject;
}

private PdfDictionary getEmbeddedDictionary (PdfFileSpec fileSpec) {
    return ((PdfDictionary) fileSpec.getPdfObject()).getAsDictionary(PdfName.EF);
}

and then called without having to check for anything else but null:

PdfDictionary fileSpecObject = getFileSpecDictionary(getEmbeddedFiles());
if (fileSpecObject == null) {
    return null;
}
try {
    PdfFileSpec fileSpec = PdfEncryptedPayloadFileSpecFactory.wrap(fileSpecObject);
    ...
}

I noticed that fileSpec.getPdfObject() did not check for isDictionary, that looks potentially like a problem? Or perhaps it's guaranteed, I wouldn't know.

One thing that's still missing from the refactored version is the logger, that should very likely be static final and not be constantly fetched (or at least that's the pattern I'm familiar with). Also, I'm kind of expecting that a call logger.error(e) or logger.error("Something went wrong", e) would Do What I Mean and not require the getMessage() call there. Passing the exception will then also allow you to see stacktraces etc., overall making that a nicer experience. (That's mostly in reference to SLF4J, but that's a guess at what library's used here.)

I'm struggling with the name getPdfStreamUf, but I suppose that's a technical bit that the reader more familiar with the format would understand.

Okay so with those changes it would be a tiny bit more linear:

public PdfEncryptedPayloadDocument getEncryptedPayloadDocument2() {
    if (readerIsEncrypted(getReader())) {
        return null;
    }
    if (collectionIsNotViewHidden(getCatalog().getCollection())) {
        return null;
    }
    PdfDictionary fileSpecObject = getFileSpecDictionary(getEmbeddedFiles());
    if (fileSpecObject == null) {
        return null;
    }
    try {
        PdfFileSpec fileSpec = PdfEncryptedPayloadFileSpecFactory.wrap(fileSpecObject);
        if (fileSpec == null) {
            return null;
        }
        PdfStream stream = getPdfStreamUf(getEmbeddedDictionary(fileSpec));
        if (stream == null) {
            return null;
        }
        return new PdfEncryptedPayloadDocument(stream, fileSpec, getDocumentName());
    } catch (PdfException e) {
        logger.error(e);
        return null;
    }
}

For me the upside is that while this is pretty heavy on the boilerplate, each individual step is pretty clear, they also have clear names and wherever we don't expect nulls, well, we just call methods and use their return values and have the proper types anywhere (no casts visible). Oh yeah and I also backtracked on the small methods issue. Actually it's pretty readable and the return null statements clearly delimit the steps.

Regarding the get.get.get chains: It might be worth exploring whether domain specific wrapper classes would help, separate from the business logic here, by exposing the right methods, hiding most of the null checks and focusing on letting the logic here use expressive accessors. That's somewhat simulated here by the get(get()) invocations, e.g. getPdfStreamUf(getEmbeddedDictionary(fileSpec)) really wants to look like fileSpec.getEmbeddedDictionary().getPdfStreamUf().