https://issues.apache.org/bugzilla/show_bug.cgi?id=46905
--- Comment #32 from Andreas L. Delmelle <[email protected]> 2009-06-26 12:40:51 PST --- (In reply to comment #31) Hi Vincent, > I've had a quick look at your patch. I have 2 small comments: > - there are two compilation errors: one in KnuthPenalty about an unknown > PENALTY_TYPE constant, one in PageBreaker, l.421, trying to convert a List > into > a LinkedList (that one is easily fixed). Hmm, sorry about those... The missing constant was an experiment of mine, to see if we could follow a similar approach as for FONode here. Instead of the fixed isBox(), isGlue() and isPenalty(), we would get something like getElementType() and isElementType(int). Undid this change for the moment, but it seems I forgot to clean up after that. The change would only make sense if we would consider adding new types of elements. In that case, we probably won't want to change KnuthElement every time to add yet another isXXX() method. The latter one I noticed earlier today, too. It was a more general change: if we do not need the LinkedList functionality, we can just as well use the List interface. > - I think you can commit the changes that are clean-up 'only' right now. They > are improving the code readability quite a bit. Good! That's the intention. Just making sure that future contributors will spend less time trying to understand the code. The goal is to make it (almost) readable by a child. ;-) > And a bit of nit-picking: > - in BlockStackingLM: in the getKeep*Property methods, I chose to throw > IllegalStateExceptions because the only LMs that don't override those methods > are LMs to which keeps don't apply. So calling those methods on such LMs is a > genuine programming mistake, and not a TODONotYetImplementedException. Initially, I had pushed the getKeep*Property() method up to the LayoutManager interface, and wanted to use a similar pattern as some standard Java interfaces. The subclass can choose to implement it, but if it does not, it is allowed to signal this with an UnsupportedOperationException. It would indeed be a mistake, just like it is a mistake to call remove() on an arbitrary Iterator, because remove() is an optional operation. A concrete iterator is not obliged to implement it, and if it doesn't, it should throw an exception. Whether it's an IllegalStateException or an UnsupportedOperationException is really all the same to me. Both are unchecked runtime exceptions. Just found the latter more appropriate... > - there's no reason to make the PageBreakingAlgorithm class public Good catch! A reminder for me to try to get around to finishing the fo:inline-container implementation. The origin of that change is that an InlineContainerBreaker would need to have access to the PBA, from within the layoutmgr.inline package. > - in PageBreakingAlgorithm.createFootnotePages: tmpLength can be declared > inside the while loop No idea whether it's still relevant, but I always tend to avoid stuff like: while (someCondition) { int intVar = intValue; ... } Instead, use: int intVar = -1; while (someCondition) { intVar = intValue; ... } which, I guess, is almost equivalent to: while (someCondition) { final int intVar = intValue; ... } apart from the fact that the variable is available outside of the loop. IOW: loop only the assignment, not the declaration. There really is no reason to declare (=allocate space for) a new variable on every iteration. Maybe using the final modifier would work here too, since we don't need the variable scoped outside of the loop... > - I see you changed the 'while (iter.hasNext())' loops into 'for (Iterator > iter > = list.iterator(); iter.hasNext();)' and... I just wanted to say that it's > great ;-) Cool! :-) > > I'll try to have a look at the bigger changes in [Page]BreakingAlgorithm later > on. OK, thanks for the feedback so far! Andreas -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug.
