On 3/31/06, C. Grobmeier <[EMAIL PROTECTED]> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Hey all, > > i have just uploaded this: > http://www.grobmeier.de/commons-compress-draft-4.zip > > Tar, Zip and BZip2 is now implemented by the 2 new interfaces. > Please check it out, and tell me, if something more i have to do > before it can be comitted to the compress-code. > > If this looks ok, i will create a bug in bugzilla and add this zip > as attachment. > > Looking forward to read your comments- > Cheers > 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]
