xmlgraphics-fop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vincent Hennebert <vhenneb...@gmail.com>
Subject Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Date Mon, 16 Feb 2009 17:29:41 GMT
Hi Jeremias,

Jeremias Maerki wrote:
> On 12.02.2009 13:35:57 Vincent Hennebert wrote:
>> And that goes on...
>>
>>> Author: jeremias
>>> Date: Wed Feb 11 14:54:26 2009
>>> New Revision: 743351
>>>
>>> URL: http://svn.apache.org/viewvc?rev=743351&view=rev
>>> Log:
>>> Added missing default font setup.
>>>  
>>> +    /** {@inheritDoc} */
>>> +    public void setDefaultFontInfo(FontInfo fontInfo) {
>>> +        FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo, getUserAgent());
>>> +        setFontInfo(fi);
>>> +    }
<snip/>
> 
> I'm so completely lost. I have absolutely no idea what you want to say
> here. I don't see any optimization potential here at all. But I'm sure
> you could do it better.

The block of code inside setDefaultFontInfo is defined three times (at
least). That’s two times two much. Please have a look at the attached
patch: is there anything wrong with it?

Of course it’s still not ideal: the font-specific methods should be
extracted into some utility class, inside some font package. But I’ve
focused on the DocumentHandler hierarchy only, and tried to fix what was
wrong in it. I don’t have the time to look at the bigger picture
unfortunately.

I’m not familiar with the font code, but I’m wondering about the
necessity of this setDefaultFontInfo method in the first place. Why
would DocumentHandlers need to be asked, by external objects, to setup
their defaut fonts? Can’t they do that by themselves, at initialization
or something?


>> Looking further into the ABWIFDocumentHandler hierarchy, we can see how
>> the startDocument method is implemented:
<snip/>
>> - in TIFFDocumentHandler:
>>         try {
>>             if (getUserAgent() == null) {
>>                 throw new IllegalStateException(
>>                         "User agent must be set before starting PDF generation");
>>             }
>>             if (this.outputStream == null) {
>>                 throw new IllegalStateException("OutputStream hasn't been set
>> through setResult()");
>>             }
>>             [...]
>>         } catch (IOException e) {
>>             throw new IFException("I/O error in startDocument()", e);
>>         }
>>
>> Note the copy/paste errors, which means that only 2 out of the 5 error
>> messages are correct.
>> There’s no TODO statement anywhere, so I assume that this code is
>> considered to be finalized. IIC this is new code: no legacy here, no
>> inherited burden.
> 
> Fixed. Thanks for pointing out the oversight.

That’s much better, but I think the IOException catch block could also
be factorized. See attached patch (I didn’t fix indentation so that
changes appear more clearly). Does it make the code any less readable?


>> Now I have just one question: why? What benefit does this development
>> method bring that I’ve missed, and that overcomes all of the well-known
>> drawbacks?
> 
> Two questions actually. But I don't know how/what to repond to that.

Well, that’s problematic then. That means that you will continue to do
that, and I will continue to complain, and that will lead us nowhere. Am
I too picky? I’m happy to be proven wrong, if I’m given reasons. But
frankly, why should we be happy with duplicated code in the codebase?
Why does the fact that we refuse copy/paste programming make it “high
coding standards”? This copy/paste error is a perfect illustration IMO.
(Note that I don’t blame you for the copy/paste error itself, I’m the
first one to make such errors.) I think copy/paste should be virtually
banned in the context of code development. I quite like the “once and
only once” argument of extreme programming actually [1].

And yes, even a 5-line block of duplicated code should be considered as
highly suspicious IMO. That often reveals a higher-level design flaw.
And in the present case, I think it can be easily avoided without making
the code any more complicated. And without spending much more time on it.

We have to agree upon something. Otherwise, peer review won’t bring its
usual benefits.

Vincent


[1] http://my.safaribooksonline.com/0201844575/ch19lev1sec5

Mime
View raw message