commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Torsten Curdt" <tcu...@apache.org>
Subject Re: [compress] Draft 7
Date Sun, 04 Jun 2006 10:07:41 GMT
Chris,

o) The usage of the "Archiver" is still awkward. You create an
Archiver, then set the filename of the archive. Why not have an
"Archive" interface representing the archive  and a "ArchiveFactory"

  Archive arch = ArchiveFactory.getInstance(new File("my.tar");

o) The Archive(r) interface lacks a delete. but you have that in the TODO :)
o) The ArchiverFactory uses too much reflection-magic:

- Using reflection for instantiation is nice as you can easily add new
implementations ..if it's likely to have many custom implementations.
I don't think that's the case here ...and you could easily extend the
ArchiveFactory to add your own implementation. So I think reflection
is not of much use here. Only gives a unnecessary overhead.

- Also calling the methods via reflections is not necessary. If you
need the methods make them part of the interface. I suggest to add a
method "isArchive(FileInputStream)" to the "Archive" interface and
move the checking into the Archive implementations. (or
AbstractArchive). So you delegate the evaluation to the
implementation.

o) Having multiple (De)CompressorFactories with different names is not
really nice. I suggest only to have that one (De)CompressorFactory.

- then "getInstance()" needs a parameter to select the implementation.
I suggest to use a similar detection scheme as for the ArchiveFactory.

  Compressor compressor = CompressorFactory.getInstance(new File("file.bz2"));

To guess the compressor from the file name.

  Compressor compressor = CompressorFactory.getInstance("bzip2");

To make it easier to select the (de)compressor when the type of
compression is only available at runtime.

o) If you want a certain compressor without going through the factory I think

  Compressor compressor = Bzip2Compressor.getInstance();

would be the right way to go.

o) I would get rid of the "Entry" in the "ArchiveEntry" methods

getEntryName() -> getName()
getEntryStream() -> getInputStream()

I don't really like/understand the setters. Why do we need them?
Oh ...and maybe there also would be the option to replace

  arch.add("myinput", new FileInputStream("input"));

with

  arch.add(new ArchiveEntry("myinput", new FileInputStream("input")));

WDYT?

I hope my suggestions don't come across too negative.
Thanks for all your work. It's really getting there! :)

cheers
--
Torsten

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