Mailing-List: contact dev-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Developers List" <dev@commons.apache.org>
Received-SPF: neutral (athena.apache.org: local policy)
Message-Id: <E88AEEBC-30A2-4070-9B17-D883C299D061@apache.org>
From: Torsten Curdt <tcurdt@apache.org>
To: "Commons Developers List" <dev@commons.apache.org>
In-Reply-To: <4858060F.5030207@apache.org>
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0 (Apple Message framework v924)
Subject: Re: [compress] A few comments
Date: Wed, 18 Jun 2008 03:06:23 +0200
References: <48577DC1.5000104@apache.org>
 <ded132f10806170214s2ea9ae63v631e39337a618e30@mail.gmail.com>
 <4858060F.5030207@apache.org>

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