After looking more closely at this I think I may have been a little hasty in my praise and Jeremias
has a point, but Max's sentiment is a right one.
On the subject of readability, there is much cleaning up and refactoring that needs to be done to
make the code base more simple and easy to understand. This is especially the case in the areas of
layout (LayoutManager implementers) and rendering. Below is a list of java classes and their
current line count in trunk.
> find ./src/java -type f -name \*.java | xargs wc -l $@ | sort -n -r
2786 ./src/java/org/apache/fop/fo/FOPropertyMapping.java
1890 ./src/java/org/apache/fop/render/afp/AFPRenderer.java
1848 ./src/java/org/apache/fop/render/pdf/PDFRenderer.java
1829 ./src/java/org/apache/fop/layoutmgr/inline/LineLayoutManager.java
1822 ./src/java/org/apache/fop/svg/PDFGraphics2D.java
1726 ./src/java/org/apache/fop/render/rtf/RTFHandler.java
1724 ./src/java/org/apache/fop/render/ps/PSRenderer.java
1666 ./src/java/org/apache/fop/pdf/PDFFactory.java
1662 ./src/java/org/apache/fop/render/pcl/PCLRenderer.java
1631 ./src/java/org/apache/fop/fonts/truetype/TTFFile.java
1597 ./src/java/org/apache/fop/layoutmgr/BlockStackingLayoutManager.java
1315 ./src/java/org/apache/fop/fonts/Glyphs.java
1291 ./src/java/org/apache/fop/layoutmgr/inline/TextLayoutManager.java
1173 ./src/java/org/apache/fop/area/AreaTreeParser.java
1144 ./src/java/org/apache/fop/layoutmgr/BreakingAlgorithm.java
1118 ./src/java/org/apache/fop/fo/Constants.java
1107 ./src/java/org/apache/fop/cli/CommandLineOptions.java
1087 ./src/java/org/apache/fop/pdf/PDFDocument.java
1085 ./src/java/org/apache/fop/layoutmgr/BlockContainerLayoutManager.java
1022 ./src/java/org/apache/fop/render/xml/XMLRenderer.java
1017 ./src/java/org/apache/fop/render/java2d/Java2DRenderer.java
902 ./src/java/org/apache/fop/render/pcl/PCLGenerator.java
892 ./src/java/org/apache/fop/layoutmgr/PageBreakingAlgorithm.java
892 ./src/java/org/apache/fop/layoutmgr/AbstractBreaker.java
We currently have Checkstyle set FileLengthCheck to a maximum of 2000 lines. Of course I realize
there is a necessary complexity in the processing task, but IMO a class shouldn't really be more
than say 500-600 lines long.. otherwise its just doing too much and should be delegating to another
class. I would be interested to hear your thoughts on this.
Adrian.
Andreas Delmelle wrote:
On Jun 9, 2008, at 16:40, Jeremias Maerki wrote:
Frankly, I'm less than thrilled. I appreciate the good will behind this
but I'd have appreciated some advance warning, too. My concern is that:
- ListElement last = (ListElement)contentList.getLast();
is much easier to read and write than:
+ ListElement last = (ListElement) contentList
+ .get(contentList.size() - 1);
When working with element lists constructs like the above are everywhere.
A linked list IS the most efficient data structure for element lists. I
generally support using the generic type instead of the specific class
(i.e. List instead of ArrayList), but LinkedList is adding some often
used methods for element lists which improve code readability. I'm
curious what other committers think about this.
Unfortunately for Max maybe, but I agree with this assessment.
In general, I have no objections to switching to the interface where it
is applicable/beneficial. As a rule of thumb, I'd reserve this approach
for any part/class in FOP that has potential use for external libraries.
Don't confuse this with simple 'public' visibility: a lot of methods are
made public, because they need to be accessible from within other FOP
sub-packages, but that does not mean that they are part of the public
API. Similarly, some methods may be marked as 'protected', but still
they would be visible to subclasses outside of FOP. So, this needs to be
determined for every occurrence separately.
For anything that remains FOP-internal, using concrete implementations
does not only improve code-readability and -writability, but IIC, it
also generates more efficient byte-code in the end. At run-time, the
interpreter would spend slightly less time on determining the actual
implementation of a method to be used.
For the above example, the interpreter originally only needed to check
the available implementations of getLast() (defined in the concrete
class LinkedList), while after the change, it would need to check those
of get() (defined in the List interface). Checking for the
implementations of interface methods is known to be slightly more
demanding than for class methods.
Also, if we always know that the concrete type of the List in question
is a LinkedList, index-based access is generally taken to be a bad idea.
list.getLast() is likely to outperform list.get(list.size() - 1),
especially if the lists grow large....
Painful lesson, maybe, but try to be mindful about changes across many
classes/packages, especially if they concern more than just
documentation or styling. It never hurts to check with the other devs
before committing. If I find myself in such a situation, I'm inclined to
attach the patch to a Bugzilla entry first, and give everyone a chance
to comment in on the proposed changes. If no feedback comes in after a
few days, you can reasonably assume that the other committers are OK
with it.
Cheers
Andreas
