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 Tue, 17 Feb 2009 17:00:08 GMT
Hi Jeremias,

Jeremias Maerki wrote:
> 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...
<snip/>
>> 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 think it’s a good illustration of what I was saying about design flaws
BTW. The method to define was probably not a setDefaultFontInfo, but
rather a getDefaultFontCollections. A small change in the point of view
that may avoid troubles in the future.

(Abstraction made of what’s said below. If no method is needed at all in
the first place...)


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

Well if the method() -> doMethod() practice is constantly used then it
becomes an easily recognizable pattern IMO. Of course, if it has to be
used absolutely everywhere... Grey area.


> But I'm not totally against it.

And I won’t push too much for it. The change you made was the most
important one.


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

And for my taste, you’re too much of a pragmatist :-P


> 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 appreciate that there’s no easy answer to that. It’s becoming
philosophical (and nothing personal, BTW), but I really, really disagree
with that whole approach. I think it’s the curse of the software
industry. That’s the cause for common people to now say: “The device
doesn’t work? Is there software inside? Then that’s the problem.” Look
at the smartphone market for example. Look at which one is rapidly
taking market shares, because it’s so much more stable and simple to
use. I believe that to a certain extent that reflects the development
policy that’s behind.

That short-term strategy will bite us back, if it hasn’t already. We
will keep implementing cheap solutions, adding workarounds on top of
workarounds, and one day we will realise that it’s impossible to
implement any new feature without re-writing the whole thing. And then
you will lose customers because they can’t imagine that such a small
feature that they’re asking for takes so much time to implement. And
then years will pass without any new version to be released, and people
will start believing that the project is dead. Sounds familiar? I’ve
never had any satisfiable answer to 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?

Quite on the contrary, IMO. ATM unpaid contributors have to deal with
a monolithic block of code with plenty of bizarre things everywhere.
Before doing any productive work they have to understand what are the
workarounds, and how they will work around those workarounds. Victor had
a major point with his modular architecture actually. I firmly believe
that this is the reason why we don’t have more contributors. The
complexity of XSL-FO only comes second, as there are whole parts of the
code that don’t deal with it. Again I’m happy to be proven wrong.


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

Well those shouldn’t even have made their way into the codebase in the
first place. Permanent refactoring, another idea of the extreme
programming approach.


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

It would be good indeed if other committers would speak up, and if we
agreed on some policy. Just agreeing upon checkstyle and pmd rules we
want to follow would already be a good start. Without converting the
whole codebase straight away, we could agree to forbid any new code that
doesn’t follow the rules. I’ve started to set up my ‘ideal’ checkstyle
file some while ago, but have yet to finish it.


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

Still there would be a psychological benefit: I for myself will be more
happy to put aside those claims of mine that I haven’t managed to get
the majority to agree upon. ATM it’s one to one indeed.


> Thanks for your patience,
> Jeremias Maerki

And thanks for yours :-)
Vincent

Mime
View raw message