-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hello Sandy,
thousand thanks for your comments and the time you have spent for this. I will work through this all and prepare another draft soon. Thank you very much, Chris > The following is a stream of things I don't think are right in the > order I observe them while skimming the code. I've never used the > compress code so excuse any suggestions that miss the point. > > * Archiver: too many overloaded methods. Especially with setters. > Having setFoo(File) and setFoo(String) will breaks the JavaBean > property conventions and could cause problems with scripting tools. If > you're going to do something like this then use different names eg: > setDestinationFile(File) and setDestinationFilename(String) and let > the latter be a virtual property that really just creates a new File > and calls setDestinationFile(File). > > * Archive: doesn't do much, only contains one static method: > getInstnace(ArchiverType). Because it's static you cannot subclass it > so there is no way to extend it's behavior for when someone else want > to implement CabArchiver. The type-safe enum example I gave in the > other email makes this class unnecessary. > > * ArchiveType/CompressorType: make them type safe enums. Actually I > think it's a mistake to distinguish between Archive types and > Compression types. Making them separate you have one library that does > two things and I think you should treat them as one thing and do that > one thing well or have two libs that each do one thing. > > > * TarConstants: is package scoped and, with the exception of one line > in TarOutputStream, is only used by TarEntry. It looks to me the logic > in TarOutputStream that uses TarConstansts could be easily pushed into > TarEnrty and then the constants in TarConstants should be moved to > where they are used. > > * TarInputStream: skimming it I don't it. It looks like it's taking > the InputStream concept and corrupting it with the notion of many > separate streams for each file in one stream. This is confusing > because it doesn't fit the expectations of an InputStream. IMO it > should be it's something similar to an Iterator that takes a raw > InputStream and provides a way to get at metadata sequentially from > the raw InputStream. From the meta data you should be able to get an > InputStream which is just for that file in the archive. > > * TarOutputStream: Same problem as TarInputStream but with an > OutputStream. Because TarOutputStream subclasses OutputStream it makes > write methods available. But using these methods are dangerous because > if you write a different number of bytes than what was passed when > putNextEntry(TarEntry) was called you corrupt the archive. A good API > doesn't let the programmer make mistakes. I'd change this to be it's > own object type that accepts TarEntrys with all the needed to add a > file in one step that either succeeds or fails. > > * TarInputStream/TarOutputStream: I don't see why these classes should > be public. Making them package private would make me care less what > they are because they aren't part of the public API and can be changed > at will. > > > * ZipLong, ZipShort: these are both public, and I don't see why they > need to be. They are only used inside the package with the exception > of 3 places ZipShort is used as a parameter type in a public method. I > don't see why those ZipShorts cannot be converted to shorts for the > public API and both of them made package private. > > * ZipOutputStream: has the same problems as TarOutputStream. > > * compress.archivers.zip: every class in this package is public, I > don't think they all need to be. > > > * compress.compressors.bzip2: This package seems fine. I don't get why > CBZip2InputStream and CBZip2OutputStream start with the letter "C" > though. > > > * compress.exceptions: I don't get why the exceptions are segregated > to their own package. Javadoc already keeps them separated so there > isn't a need to put them in their own package. I'd put them in the > compress package or a package closer to where they are used. > > -- > Sandy McArthur > > "He who dares not offend cannot be honest." > - Thomas Paine > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2.1 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEMi31kv8rKBUE/T4RAhe3AJ9HCrNQf8dvHAyJnW20dWoF4tpnxwCgi7U8 Hu/4efxAre76d8mbK/mqPrw= =4wUL -----END PGP SIGNATURE----- --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
