https://issues.apache.org/bugzilla/show_bug.cgi?id=46705
--- Comment #7 from Vincent Hennebert <[email protected]> 2009-02-17 03:00:27 PST --- That's great new functionality. I've only had time to look at a part of the patch so far, so I'm posting my few comments below. More later if I have time. In the o.a.f.accessibility package: - why are there two classes? Only the sub-class seems to be externally used, so it may as well be merged with the super-class. If some more general functionality turns out to be necessary, the extraction of a common super-class can always be done later. - Likewise, only one constructor seems to be used ATM. If other constructors are needed, they can also be added later on. - AFAIU mTranHandler is set in all of the constructors. Why systematically test it for null in the methods then? There is an encapsulation problem in o.a.f.apps.Fop.getDefaultHandler(): the lines of code dealing with accessibility should be moved to the accessibility package. In FOUserAgent: - the "accessibility" string should be defined only once in a public final static field (ideally somewhere inside the accessibility package) - there's no need to explicitly put false for the accessibility option in the constructor, as the value is tested for null in the accessibilityEnabled method In FopFactoryConfigurator.configure(): - any reason why the accessibility option is not handled like the other options? (using getValueAsBoolean) And a nit about coding style: you seem to be using Hungarian notation to name fields ('mTheClassField'). I don't have anything against this notation, but it's not used inside the rest of FOP's codebase. For consistency we may want to agree upon a notation. FWIW I tend to think that, thanks to modern development environments that now highlight class members, this notation is no longer so useful. Thanks, Vincent -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug.
