You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-dev@xmlgraphics.apache.org by Vincent Hennebert <vh...@gmail.com> on 2009/02/12 13:35:57 UTC
Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
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);
> + }
The setDefaultFontInfo method is defined 5 times in the
AbstractBinaryWritingIFDocumentHandler hierarchy:
- one in ABWIFDocumentHandler itself, here’s the code:
FontManager fontManager = getUserAgent().getFactory().getFontManager();
FontCollection[] fontCollections = new FontCollection[] {
new Base14FontCollection(fontManager.isBase14KerningEnabled())
};
FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
fi.setEventListener(new
FontEventAdapter(getUserAgent().getEventBroadcaster()));
fontManager.setup(fi, fontCollections);
setFontInfo(fi);
- one in the AFPDocumentHandler sub-class:
FontManager fontManager = getUserAgent().getFactory().getFontManager();
FontCollection[] fontCollections = new FontCollection[] {
new AFPFontCollection(getUserAgent().getEventBroadcaster(), null)
};
FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
fi.setEventListener(new
FontEventAdapter(getUserAgent().getEventBroadcaster()));
fontManager.setup(fi, fontCollections);
setFontInfo(fi);
- one in PCLDocumentHandler:
FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo,
getUserAgent());
setFontInfo(fi);
- one in TIFFDocumentHandler:
FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo,
getUserAgent());
setFontInfo(fi);
One could think: it’s not too bad, the code is duplicated only twice, in
the last two cases it’s a one-line call to another method. Yes, but
guess what is the code of that method:
Graphics2D graphics2D = Java2DFontMetrics.createFontMetricsGraphics2D();
FontManager fontManager = userAgent.getFactory().getFontManager();
FontCollection[] fontCollections = new FontCollection[] {
new org.apache.fop.render.java2d.Base14FontCollection(graphics2D),
new InstalledFontCollection(graphics2D)
};
FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
fi.setEventListener(new
FontEventAdapter(userAgent.getEventBroadcaster()));
fontManager.setup(fi, fontCollections);
return fi;
Looking further into the ABWIFDocumentHandler hierarchy, we can see how
the startDocument method is implemented:
- in AFPDocumentHandler:
try {
if (getUserAgent() == null) {
throw new IllegalStateException(
"User agent must be set before starting PostScript
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);
}
- in PCLDocumentHandler:
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);
}
- in PDFDocumentHandler:
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);
}
- in PSDocumentHandler:
try {
if (getUserAgent() == null) {
throw new IllegalStateException(
"User agent must be set before starting PostScript
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);
}
- 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.
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?
Vincent
Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Posted by Vincent Hennebert <vh...@gmail.com>.
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
Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
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
Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Posted by Vincent Hennebert <vh...@gmail.com>.
And the attached patch >_<
Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Posted by Vincent Hennebert <vh...@gmail.com>.
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
Re: svn commit: r743351 - /xmlgraphics/fop/branches/Temp_AreaTreeNewDesign/src/java/org/apache/fop/render/pcl/PCLDocumentHandler.java
Posted by Jeremias Maerki <de...@jeremias-maerki.ch>.
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);
> > + }
>
> The setDefaultFontInfo method is defined 5 times in the
> AbstractBinaryWritingIFDocumentHandler hierarchy:
>
> - one in ABWIFDocumentHandler itself, here’s the code:
> FontManager fontManager = getUserAgent().getFactory().getFontManager();
> FontCollection[] fontCollections = new FontCollection[] {
> new Base14FontCollection(fontManager.isBase14KerningEnabled())
> };
>
> FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
> fi.setEventListener(new
> FontEventAdapter(getUserAgent().getEventBroadcaster()));
> fontManager.setup(fi, fontCollections);
> setFontInfo(fi);
>
> - one in the AFPDocumentHandler sub-class:
> FontManager fontManager = getUserAgent().getFactory().getFontManager();
> FontCollection[] fontCollections = new FontCollection[] {
> new AFPFontCollection(getUserAgent().getEventBroadcaster(), null)
> };
>
> FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
> fi.setEventListener(new
> FontEventAdapter(getUserAgent().getEventBroadcaster()));
> fontManager.setup(fi, fontCollections);
> setFontInfo(fi);
>
> - one in PCLDocumentHandler:
> FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo,
> getUserAgent());
> setFontInfo(fi);
>
> - one in TIFFDocumentHandler:
> FontInfo fi = Java2DUtil.buildDefaultJava2DBasedFontInfo(fontInfo,
> getUserAgent());
> setFontInfo(fi);
>
>
> One could think: it’s not too bad, the code is duplicated only twice, in
> the last two cases it’s a one-line call to another method. Yes, but
> guess what is the code of that method:
> Graphics2D graphics2D = Java2DFontMetrics.createFontMetricsGraphics2D();
>
> FontManager fontManager = userAgent.getFactory().getFontManager();
> FontCollection[] fontCollections = new FontCollection[] {
> new org.apache.fop.render.java2d.Base14FontCollection(graphics2D),
> new InstalledFontCollection(graphics2D)
> };
>
> FontInfo fi = (fontInfo != null ? fontInfo : new FontInfo());
> fi.setEventListener(new
> FontEventAdapter(userAgent.getEventBroadcaster()));
> fontManager.setup(fi, fontCollections);
> return fi;
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.
> Looking further into the ABWIFDocumentHandler hierarchy, we can see how
> the startDocument method is implemented:
> - in AFPDocumentHandler:
> try {
> if (getUserAgent() == null) {
> throw new IllegalStateException(
> "User agent must be set before starting PostScript
> 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);
> }
>
> - in PCLDocumentHandler:
> 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);
> }
>
> - in PDFDocumentHandler:
> 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);
> }
>
> - in PSDocumentHandler:
> try {
> if (getUserAgent() == null) {
> throw new IllegalStateException(
> "User agent must be set before starting PostScript
> 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);
> }
>
> - 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.
> 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.
Jeremias Maerki