commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "C. Grobmeier" <>
Subject Re: [compress] Draft 7
Date Mon, 12 Jun 2006 07:32:14 GMT
Hash: SHA1


> 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");

please take a look at the examples:

Archiver archiver = ArchiverFactory.getInstance(			new

You can allready do that. Further you can do this:

If you don't have one, you are going thru

And set the name with;
Please verify if you have draft 7.

> o) The Archive(r) interface lacks a delete. but you have that in the
> TODO :)

Truely :-)

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

OK, good point. I agree with with the isArchive method, that's a good
thing here. I will do that. I am a bit sorry about the "add
impl"-feature, cause i like it. But i will change this too cause it's
really a bit much of reflection.

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

Yes, all true. I will change that. I think it's a good thing to move the
Decompressor Interface methods to the Compressor interface too and to
delete the Decompressor. This is similar to Archiver, which is a nice
thing now.

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

Ok, that's easy. Why would someone like to do that?

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

We don't need them, i deleted it. I am sorry for that, it seems i am
sick with making lots of unnecessary setters. I always do that.

> 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")));

Feels good to me.

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

No problem. Sorry for my delay i was at a 5 days festival and needed one
week to come back into reality. I know that my code is not (always) the
best, so i am glad about the time you (all) spent for review. Looks like
i'm a setter-maniac, it's nice to know this :-)

- - Chris.

Version: GnuPG v1.4.2.1 (MingW32)
Comment: Using GnuPG with Mozilla -


To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message