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/
   1890 ./src/java/org/apache/fop/render/afp/
   1848 ./src/java/org/apache/fop/render/pdf/
   1829 ./src/java/org/apache/fop/layoutmgr/inline/
   1822 ./src/java/org/apache/fop/svg/
   1726 ./src/java/org/apache/fop/render/rtf/
   1724 ./src/java/org/apache/fop/render/ps/
   1666 ./src/java/org/apache/fop/pdf/
   1662 ./src/java/org/apache/fop/render/pcl/
   1631 ./src/java/org/apache/fop/fonts/truetype/
   1597 ./src/java/org/apache/fop/layoutmgr/
   1315 ./src/java/org/apache/fop/fonts/
   1291 ./src/java/org/apache/fop/layoutmgr/inline/
   1173 ./src/java/org/apache/fop/area/
   1144 ./src/java/org/apache/fop/layoutmgr/
   1118 ./src/java/org/apache/fop/fo/
   1107 ./src/java/org/apache/fop/cli/
   1087 ./src/java/org/apache/fop/pdf/
   1085 ./src/java/org/apache/fop/layoutmgr/
   1022 ./src/java/org/apache/fop/render/xml/
   1017 ./src/java/org/apache/fop/render/java2d/
    902 ./src/java/org/apache/fop/render/pcl/
    892 ./src/java/org/apache/fop/layoutmgr/
    892 ./src/java/org/apache/fop/layoutmgr/

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.


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.



Reply via email to