-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Cheers,
> 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
File("C:\\Temp\\ZIPTEST.zip"));
You can allready do that. Further you can do this:
If you don't have one, you are going thru
ArchiverFactory.getInstance("zip");
And set the name with Archiver.save(File);
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()
>
OK.
> 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.
> WDYT?
>
> 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFEjRhxkv8rKBUE/T4RAsrjAJ479tE0zqTkkNdmszqVJVBra5moxQCfbfj0
bqyU1USmLIVhiryrVhziKF8=
=fAAt
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
