You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Torsten Curdt <tc...@apache.org> on 2006/06/01 12:48:46 UTC
Re: [compress] Draft 7
I will have a look over the weekend. Thanks for the work.
cheers
--
Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [compress] Draft 7
Posted by Torsten Curdt <tc...@apache.org>.
> > 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.
IMO you add a file to the individual "Archive" not to the "Archvier".
That's why I would prefer to have a "TarArchive" that represents the
original archive file. Its name would be immutable.
So I suggest
o Archiver -> Archive
o make the Archive name immutable (no setName)
But maybe I am off here ...any other opinions?
> 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.
Hm... I liked the separation ...but I fear you are right - will there
at any stage be a compressor without the decompressor? That's the
questions. Answer: it probably should not. So it might be ok...
What do others think?
> > 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?
Think you are right - direct instanciation is probably good enough.
Compressor compressor = new Bzip2Compressor();
Question is whether compressor is threadsafe and one instance could be
shared ...but fair enough. Probably better that way.
Thanks!
cheers
--
Torsten
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [compress] Draft 7
Posted by "C. Grobmeier" <gr...@possessed.de>.
-----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: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [compress] Draft 7
Posted by Torsten Curdt <tc...@apache.org>.
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