Hi Jeremias,

I’d have a couple of comments regarding your changes. I’ve only had 
a quick look and don’t understand this part of the code well, but 
following the principle of pair review, here they are:

- at several places there is code like the following:
    private void checkInTextObject() {
        if (!inTextObject) {
            throw new IllegalStateException("Not in text object");
  I understand and don’t criticise the need for safety checks, 
  especially if the code is new or complex. Only, since we now have 
  Java 1.4 as a minimal requirement, why not use assert statements? They 
  were created exactly for that.

- in PDFTextUtil:
  - the setTextRenderingMode(int) method is only used within 
    PDFTextUtil; why then not making it private? The other 
    setTextRenderingMode method will still be available and seems to be 
    much safer to use anyway.
  - it seems to me that a bit more of the internal state of 
    a PDFTextUtil object could be controlled by the object itself, 
    especially in the following piece of code (PDFTextPainter, l.220):
        Font f = textUtil.selectFontForChar(ch);
        if (f != textUtil.getCurrentFont()) {
    A signalFontUsed(f) method might make sense. Basically doing the 
    same test as above, but inside PDFTextUtil. The code would be better 
    encapsulated, so more maintainable. WDYT?

- in PDFTextPainter: now that the code is working, you probably want to 
  remove the DEBUG variable and all the statements displaying stuff to 
  System.out; or replace them with proper log.debug statements ;-)

For the rest I believe when you say it’s working... Congrats!


Reply via email to