Hi
I'm going to address Dave's three mails in a single response
dam6923 . wrote:
> In SevenZFile.java
>
> Constructor...
> 1) Close file on exception instead of the current technique of keeping
> a "succeeded" flag.
This means I have to catch and rethrow the exception. I don't think I
like this better than the flag.
> 2) Move setting the password to the last line of the constructor so
> that it isn't stored if an exception is thrown at any point.
As the code stands this won't work as readHeaders needs the password
field. But we probably should store the password as a char[] anyway and
clear it when done - I'll change that before cutting the next release.
> General...
> 1) I see a few instances of
> if (file != null) {
> try {
> file.close();
> } catch (IOException ignored) { // NOPMD
> }
> file = null;
> }
> Can we snag the the IoUtils close methods in this project?
yep, but after the 1.6 release.
> 2) The class JavaDoc comments are wrapped after about 50 characters..
> extend to 80?
It may look wierd, but does it hurt?
> 3) There's a 'leaked resource' warning on line 190. A
> ByteArrayInputStream is not being closed, which makes sense as closing
> it does nothing, but for completeness sake perhaps or to protect
> against a time that the input-source could change and then it really
> is leaking.
> 4) Non-standard logging. There are many logging statements in the
> code... perhaps delegate that to a known logging framework or remove
> it all entirely?
> 5) Line 870 - magic number
will fix before the release (and remove the logging).
> org.apache.commons.compress.compressors.lzma.LZMACompressorInputStream#read()
>
> Current: count(ret == -1 ? -1 : 1);
> Change: count(ret == -1 ? 0 : 1);
doesn't make a difference if you look at count's implementation. Will
change it anyway.
> org.apache.commons.compress.archivers.ar.ArArchiveInputStream#getNextArEntry()
>
> The code to "skip to the next available entry" looks a bit inefficient
> because it reads one byte at a time in a loop. Consider using
> org.apache.commons.compress.utils.IOUtils.skip(InputStream, long)
>
> Consider using
> org.apache.commons.compress.utils.IOUtils.readFully(InputStream,
> byte[], int, int) on line 98 or else there may be an unnecessary
> exception thrown.
>
> In fact, everywhere there is a read for header information, consider
> using "readFully". Right now, lines 118-124 are not doing readFully
> and the return value is not being checked.
I agree we could improve this, but I'd prefer to keep the changes before
cutting the next RC down to a minimum. So "yes, but for 1.7".
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
