commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sandy McArthur" <sandy...@apache.org>
Subject Re: [compress] Interface is ready
Date Sun, 02 Apr 2006 19:48:37 GMT
On 3/31/06, C. Grobmeier <grobmeier@possessed.de> 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: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message