xmlgraphics-fop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeremias Maerki <...@jeremias-maerki.ch>
Subject Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Date Mon, 16 Feb 2009 23:38:41 GMT
Hi Vincent

On 16.02.2009 18:29:41 Vincent Hennebert wrote:
> 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.

Thanks, now I understand where you want to go.

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

Good point. I guess they could do that. This evolved over time and I
focused on different things so I didn't notice that it could be done in
a better way.

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

That's startDocument() -> doStartDocument(). That would have to be done
for all methods in the IFDocumentHandler and IFPainter in all
implementations that produce an output stream. I'm a bit sceptical as to
what that does to code readability. But I'm not totally against it.
It just takes some time to do and right at this moment, it might not be
the right moment to do that as it could have an impact on Jost's patch.
After that has been processed, it will be less problematic to do. 

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

Ok, that's more concrete to answer to. Are you too picky? For my taste,
yes. You're a purist. You want clean, beautiful code. Basically nothing
wrong with that by itself. I on the other side am a pragmatist. I need
to get things done. Function points over beauty. I fix things when they
occur. Probably nothing wrong with that by itself, too.

My clients' primary concern is, for example, that they get a very fast
intermediate format. Of course, there's also a certain interest to keep
things maintainable. I believe the new output implementations are a big
step forward to make it easier to add new output formats or to extend
existing ones (compared to the Renderer implementations).

My clients pay me money for implementing things. If it takes too long to
implement something (i.e. costs too much), the client won't accept the
offer (and might just buy a commercial license of another FO processor)
or I have to invest more of my unpaid time to compensate which means I
earn even less than I already do in some contracts. If you have an
employer that lets you work on FOP for just how long it takes until
you're happy with the code (or if you have enough energy left after work),
that's good for you. I'm not in that position. I always have to make
compromises. Maybe the reality sits in between our two positions. I'm
trying to concede to your view as far as is possible. But if this
project ran on your concept, I'd have to leave. It's as simple as that.
I don't write that to criticize you or to put you under pressure. It's
just economics. Or take people like Andreas who, if I'm right, does this
FOP thing totally in his free time (for fun I guess). Can they keep up
to your standards with the little time they have available to help
advance the project? You know we had a similar discussion face to face a
while back. We're still on two different sides of the river and I'm not
sure how we can bridge that.

Once and only once: Great ideal. I totally agree. But IMO, just not
always realistic.

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

As you've seen, sometimes I don't recognize opportunities. That's where
additional pairs of eyes are great. But even if I did, it can take a lot
of time to actually address them, even if you think otherwise. If I do
the font stream refactoring (which took me a large part of my Monday
morning), on whose time do I do this? Do I bill my client? Does he
approve if I do that? Do I do it on my own time and lose income? Can I
afford to do that? If I refactor the font stream, do I also address
these other two or three non-beauties I found while scanning through the
code? How long will that take me? Where do I stop? These are the
questions I have to ask myself.

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

We can agree to disagree and try to let each other live. Given our
different expectations, peer review may also mean different things to us.
While one thinks that something definitely should not be done like that,
another might think that, while not optimal, it's good enough. I don't
know. Maybe the FOP committers need to discuss and agree on a policy
what the project expectations are, the main project values. Surely a
difficult and controversial thing to do but maybe necessary so we two
don't clash together every now and then. Maybe the result means that I
have to invest more energy (or take other consequences) or you have to
lower your expectations (and maybe take consequences of your own). At
any rate, I believe this cannot be solved between us two. It's something
the FOP developer community has to agree on. Most likely, it won't make
everybody happy. But that's an impossible thing to do anyway.

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



Thanks for your patience,
Jeremias Maerki


Mime
View raw message