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] New draft 5
Date Sun, 30 Apr 2006 20:04:23 GMT
On 4/30/06, C. Grobmeier <grobmeier@possessed.de> wrote:
> here is a new draft for the compress interface:
>
> * http://grobmeier.de/commons-compress-draft-5.zip

Same as before, suggestions as a stream of consciousness, take the
ones you like:

package org.apache.commons.compress:

In Archiver:
Why do pack() and packToFilename(String) both return a boolean and
throws a PackException? The Javadocs state "@return true, if the
operation has been ended without exceptions". IMO the return type
isn't meaningful as if there was an exception that would be thrown,
and a return of false would never actually happen. When is it
meaningful to swallow an exception and still return false?

Does the Archiver ever unpack to a file or only a directory? In Java a
directory is represented as a File but I think it would be nice if the
Javadocs made that clear for getUnpackDestination(),
setUnpackDestinationName(String), and setUnpackDestinationFile(File).
And maybe those setters should throw an exception when the parameter
isn't a directory.

I don't really like that the Archiver interface combines both both
pack and unpack and there are difference sets of  properties for each
operation. In my mind you have the concept of an Archive and then you
ask for another implementation of "Exploder" and "Packer" interfaces.
Each one only does one thing and the Archiver can choose to make both
types available, only make one type available, support both types but
throw an exception when the second type is requested to enforce one
activity at a time.

In Compressor:
I don't see a need to expose that a FileInputStream is being used
internally. Someday you may want to implement an optimization that
decompresses small files totally in memory. Change:
FileInputStream streamCompressedInputStream(FileInputStream input),
FileInputStream streamCompressedFilename(String pathToFile), and
FileInputStream streamCompressedFile(File input) to return an
InputStream instead.

I also don't like that Compressor mixes compress and decompress
specific behaviors in one interface but its not so bad because there
are not properties that go with a behavior. All relevant information
is included in the method call parameters which is good.

In ArchiverType:
public static ArchiverType ZIP = ...; and public static ArchiverType
TAR = ....; should both be final.

In AbstractCompressor:
The copy( final InputStream input, final OutputStream output ) method,
while useful, doesn't really belong in AbstractCompressor. It's just a
utility method that an implementation may need. IMO it doesn't warrant
being part of the exposed API. It's nice to aid in code reuse but I
don't think it should happen as the expense of sticking to one
purpose.

Since Java 1.2 the File class has had createTempFile methods which
makes the ones in AbstractCompressor unneeded and, while unlikely, it
is possible that createTempFileName() could return the same name in
two successive calls which would be bad.

In CompressorType:
public static CompressorType BZIP2 = ... ; also, final.

For both of ArchiveType and CompressorType add a valueOf(String)
method. This is what Java 1.5 Enums have and it lets you convert a
String, say from a config file, into an Enum. To do this right you'll
need to keep track of other Types with a Map or something in the
constructor.

In CompressException, DecompressException, PackException, and UnpackException:
The constructors that takes (String, Exception) call
this.setStackTrace( e.getStackTrace() ); If Java 1.4 is a minimum
requirement than loose that as it's misleading and use the initCause
method to do exception chaining.

packages org.apache.commons.compress.archivers.tar, and
org.apache.commons.compress.archivers.zip:

These packages have a minimally exposed API and I like that. This will
help make future improvements easier to implement.

package org.apache.commons.compress.compressors.bzip2:

Same comments as before about the Bzip2{In,Out}Streams which you have
in the TODO.txt file.

> This draft is not perfect, as you can see in the todo list. But imho we
> have a quite good base for future work, everything compiles and works
> and is documented. So i would like to propose that someone is comitting
> this, except you have reasons not to do this.
> If you agree with me, i will open a bugzilla issue and add the link to
> the code.

I feel like the public interface was noticeably more usable and has
less noise. IMO it's not yet a great API but it's a very decent one.

--
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