On Fri, Nov 1, 2013 at 1:50 AM, Vincent Hennebert <vhenneb...@gmail.com>wrote:

> 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<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.
>

No thank you. The original code is broken since it depends on use of a
ClassCastException when it shouldn't. I only fixed this because findbugs
reported this problem and whomever checked in in the first place didn't fix
the warning correctly.

I have recently pointed out on a couple of occasions that people are
leaving checkstyle or findbugs warnings in the tree and not fixing them. If
they don't fix them in the first place, then I'll do it when I do a commit
in order to get a clean tree. If they want to go back and revise my fix,
that's fine with me as long as they don't leave warnings lying around.


>
> Thanks,
> Vincent
>

Reply via email to