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] A few comments
Date Wed, 18 Jun 2008 01:06:23 GMT
> - the exceptions could extend IOException

Could - but why restrict it that way? (composition over inheritance)

> - I'm not sure to understand the purpose of the jar classes, isn't a  
> jar just a renamed zip ?

Essentially - yes. But the JDK also handles them differently because  
of meta data and so on.

> - what's the intent with the empty classes CompressorInputStream and  
> CompressorOutputStream ? The GzipCompressor*Stream would become  
> unnecessary without these.

I think it's a relict from the factory code. But I am not sure it  
would be so bad to keep them (for now) for the symmetry of the API.
If we *really* don't need them - getting rid of that could be the last  
thing we do before a release ;)

> - would it make sense to put the unix permissions at the  
> ArchiveEntry level ? Or maybe have a base class (UnixEntry?) for the  
> Tar/Ar/ZipEntry classes ?

Well, I think e.g. RAR carries slightly different permissions.  
UnixEntry might be idea though.

> - hungarian notation must die :)

Yepp!! ....these classes are borrowed from ant :)

> - IOUtils.copy(in, out) could call copy(in, out, size)

Where is that?

> - the exception handling in the factories could be simplified

+1

> - the header in ArArchive*Stream could be factorized (ArConstants?  
> along with the sizes of the entry fields)

Hm ...IMO only useful if used somewhere else. Otherwise it's just  
work. Or where do you see the benefit?

> - it seems there are some duplicate classes in the zip package  
> (ZipEntry vs ZipArchiveEntry)

Yepp ...ups!

> - Ant has a JarMarker class that extends ZipExtraField, is there an  
> equivalent ?

Could you elaborate?

> - the zip package seems quite complicated, do we have to expose all  
> these classes as public ?

Would actually be good to hide some more of that. Indeed.

> - I wonder if the changeset model is mature enough.

No - it's not :)

> A higher level API to manipulate the archives would be nice, but I  
> believe an initial release could be made with just the stream classes.

Hm ...indeed. But IMO the changeset stuff is where it actually gets  
interesting :)

> - for simplicity, I would rename the 2 factories as Compressors and  
> Archives. The name of the methods could be shortened too, maybe  
> something like Compressors.newStream("bzip2", in).

Not a big fan of static factory accessors. For the length - sure we  
could rename them. On the other hand the current names are quite self  
explanatory. Like that.

> - what is missing to complete the API ? The implementation of the  
> changes package ? Or did you plan anything else ?

Mostly changeset support and cleaning up. Especially the testcases  
need some more love.

cheers
--
Torsten

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message