In response to your six comments/questions: 1. I did not make the private method generic enough to work for any image. It works for those images that are being processed by the drawImage() method that might call CreateAlphaMask(). As such, at that point in the process the getPixels() method is guaranteed to return an int[]. If not, the ClassCastException would have been thrown when the call to getInstance() had occurred. So, basically, I handled it consistent with how it appeared to me the rest of the code was dealing with getPixels().
2. Similar to the previous question, the images have already been put into standard RGB format so the alpha channel is in the most significant byte of the int. 3. Agreed. I hadn't really paid any attention since my IDE takes care of such spacing. 4. Agreed. The fully qualified names are there simply from doing a cut and paste from other code within iText. 5. Agreed. I wasn't particularly concerned with how the exception is record or if it is recorded. This is just a standard exception handler that my IDE will generate. If the code is adopted then there's no reason not to remove the logger call. 6. Since there is no javadoc in the rest of the code, I opted not to generate any. Steve On Dec 30, 2008, at 12:50 PM, Mark Storer wrote: > Lemme put my "code review" hat on... there we go. I hope this > doesn't come > across as harsh, but it *is* criticism (and a couple questions). > That's what > code reviews are for, and the goal is to produce better code (and > hopefully a > better coder), not to attack anyone, poke fun at them, disparage their > parenting or personal hygiene, nor to accuse them of molesting farm > animals. > > All that stuff is just a bonus. ;) > > 1 & 2 are potential bugs, the rest aren't that big a deal. > > 1) How do you know when pixelGrabber.getPixels() will return int[] > vs byte[]? > If it returns byte[], you're looking at a ClassCastException. I've > never used > a pixel grabber, so your code could be perfectly kosher, I just > thought it was > worth asking. > > 2) How do you know the order of 'colorants' in the integer? Your > code assumes > it to be in the high 8 bits of the int. Is this guaranteed? > > 3) createAlphaMask contains a mix of tabs and spaces. No Bueno. > Using nothing > but tabs is tolerable, but different editors *will* use different > widths for > tabs. Spaces will always be displayed consistently. Spaces good. > Tabs bad. > ;) > > 4) I'm surprised you needed to fully qualify > java.awt.image.PixelGrabber and > java.awt.image.ImageObserver.ABORT. ImageObserver in particular is > specifically > imported by name. > > 5) The use of Logger. createAlphaMask is the only place it's used > inside this > particular file. I checked several Important Classes (PdfReader, > PdfWriter, > PdfContentByte...) and didn't see it used anywhere. My private > branch of > paulo155 doesn't use it at all. It's not much use if no one knows > to look for > it. > > Which segues nicely into... > > 6) No javadoc. Neither do any of the other private methods in the > 'implementation' section, but it could be used to clue people into > the whole > logger thing, removing at least part of my objection to its use. > > > --Mark Storer > PDF Guy > Cardiff.com > > #include <disclaimer> > typedef std::disclaimer<Cardiff> DisCard; > > > ------------------------------------------------------------------------------ > _______________________________________________ > iText-questions mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/itext-questions > > Buy the iText book: http://www.1t3xt.com/docs/book.php > ------------------------------------------------------------------------------ _______________________________________________ iText-questions mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/itext-questions Buy the iText book: http://www.1t3xt.com/docs/book.php
