We have here a typical example of how code is being made worse due to the rigidity of a code checking tool:
Author: gadams Date: Thu Oct 31 19:44:25 2013 New Revision: 1537600 Modified: xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java?rev=1537600&r1=1537599&r2=1537600&view=diff ============================================================================== --- xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java (original) +++ xmlgraphics/fop/trunk/src/java/org/apache/fop/render/pdf/PDFStructureTreeBuilder.java Thu Oct 31 19:44:25 2013 @@ -358,7 +358,12 @@ class PDFStructureTreeBuilder implements } public StructureTreeElement startNode(String name, Attributes attributes, StructureTreeElement parent) { - PDFStructElem parentElem = parent == null ? ancestors.getFirst() : (PDFStructElem) parent; + PDFStructElem parentElem; + if ((parent != null) && (parent instanceof PDFStructElem)) { + parentElem = (PDFStructElem) parent; + } else { + parentElem = ancestors.getFirst(); + } PDFStructElem structElem = createStructureElement(name, parentElem, attributes, pdfFactory, eventBroadcaster); ancestors.addFirst(structElem);
The modified code doesn’t behave the same as the original. In the original code, if parent is not null but is not an instance of PDFStructElem, then a ClassCastException will occur. In the new code, parentElem will silently be set to ancestors.getFirst(). The original code is fail-fast: if a mistake is introduced in a later modification, such that it’s no longer an instance of PDFStructElem that is being passed, then it will throw an exception. Forcing the developer to reconsider the change, or adapt this method appropriately. With the new version, no exception will be thrown, and depending on the modification, it may or may not be correct. More likely not. And this will go unnoticed. And if it’s noticed, then a debugging session will have to be launched, and the code studied, and after a while the developer may arrive in this method, and then wonder why there is this instanceof check, what does it mean, what should be done for a non-PDFStructElem, etc. And after some (long) time being passed they might figure out that there was a mistake in their modification. I suggest you revert this change. Thanks, Vincent
