[ http://issues.apache.org/jira/browse/DERBY-826?page=all ]
Andrew McIntyre updated DERBY-826:
Andrew, thank you *so* much for adding those comments! I wish they
had been in place when I was starting to read that code.
I successfully applied your patch and confirmed your results, and
they look good to me. Here are a few comments that you might
want to consider:
1) At around line 1123 of the new Main.java, you introduced a new
variable "InputStream is = null;" for no reason that I can see.
Perhaps this new variable is unnecessary?
2) When I run your particular test case (java -cp derbynet.jar sysinfo)
with the new code, the result is a little bit confusing because
derbynet.jar now appears twice. The output looks like:
--------- Derby Information --------
JRE - JDBC: J2SE 1.4.2 - JDBC 3.0
[C:/bryan/src/derby/main/trunk/jars/sane/derby.jar] 10.2.0.0 alpha - (392542M)
[C:/bryan/src/derby/main/trunk/jars/sane/derbynet.jar] 10.2.0.0 alpha -
(392542M)
[C:\bryan\src\derby\main\trunk\jars\sane\derbynet.jar] 10.2.0.0 alpha -
(392542M)
Does this duplication of output have something to do with the direction
of the slashes in the file names? Do you understand why I got this
behavior?
3) Stylistically, I wonder if it would be clearer to take the code
which cleans up an URL for presentation, starting at around line 811
in the new file, and break that code out into a subroutine. That way,
the overall flow of loadZipFromResource(), which is an important
routine for understanding sysinfo, would be easier to follow.
4) It would be nice to beef up the comments for mergeZips slightly,
as I found myself asking two questions when I was first reading it:
- First, I didn't understand why there would be duplicates in the
arrays, so it might be nice to provide some comments explaining
why duplicate entries in the arrays might occur
- Second, I didn't understand why the second array might be null, and
I got a little bit thrown by the concept of "merging" an array with
a null array. But then I realized that the mergeZips routine *also*
can be called just to compress duplicates out of a single array.
Again, thanks very much for all the clear code and great comments in
your patch.
thanks,
bryan
