commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From will pugh <willp...@sourcelabs.com>
Subject Re: [compress] New draft 5
Date Mon, 01 May 2006 19:29:05 GMT
I may be missing some requirements here, but it seems like in light of 
the JavaBean bits we were talking about, that it makes sense to restrict 
the properties on the actual interface, and have them correspond 
directly to JavaBean properties.  In addition, I would propose getting 
rid of some properties and moving them onto the method calls.  Is there 
a reason not to?

For an interface that basically does two things, it is suprising that it 
has 16 methods on it.  Of those, 9 appear to be getter/setters on 
properties.  Those 9 getters/setters refer to 3 properties in 
AbstractArchiver.  Of those 3 properties, only 1 is used by more than 
one method (that is not a getter/setter).  Why do we have 
destinationFile and unpackDestination properties on the object, if they 
are only used in one place?  Why not just make these parameters? If you 
did this, you would have an interface that offers the exact same 
functionality, but has only 6 methods.  Seems like there is a bit of 
extra entropy.

In addition, I sorta like the  way java.io.File, java.util.zip.ZipFile, 
etc let you set the source file in the constructor.  It makes the code 
using the objects less verbose, with no loss of readability.  I would 
suggest that we add this to the archivers, by adding constructors that 
can take a string or a file for the source file.  In addition, we can 
bubble those changes up to the ArchiverType class.

What is the purpose of the ArchiverType class?  I'm assuming the idea is 
that this helps folks build factories, but that it is perfectly 
acceptable and encouraged to create any of the archivers via 'new'.  Is 
that correct?

       Thanks,
       --Will


Sandy McArthur wrote:

> On 5/1/06, will pugh <willpugh@sourcelabs.com> wrote:
>
>>
>> Sandy McArthur wrote:
>>
>> > On 4/30/06, will pugh <willpugh@sourcelabs.com> wrote:
>> >
>> >> 1)  You often change method names based on the parameter types, e.g.
>> >> Archiver.addFile + Archiver.addFileName, setUnpackDestinationName +
>> >> setUnpackDestinationFile, etc.  It seems more conventional  and less
>> >> chaotic to give all the methods the same name, and have them only 
>> differ
>> >> based on parameter.  Examples of this style are constructors for
>> >> java.utils.zip.ZipFile, java.io.FileInputStream,
>> >> java.io.FileOutputStream, org.apache.commons.io.IOUtils.IOUtils.copy,
>> >> org.apache.commons.io.FileUtils.isFileNewer, etc
>> >
>> >
>> > This is intentional, in a previous version the method names were the
>> > same. The problem with using the same name but different param types
>> > breaks the JavaBean property getter/setter rules and the classes will
>> > not be as usable in at least some scripting environments.
>>
>> Hmm.  What scripting envirionments don't allow you to call this kind of
>> Java method?  I looked at the documentation for Jythong, JRuby and
>> Groovy and they all seemed to be O.K. with dealing with methods that
>> differ by parameter types.
>>
>> Also, it doesn't seem like the current interface adheres to the Java
>> Bean specification anyways (unless I'm missing somethign).   If you
>> wanted to adhere to the Java Beans Spec, I would suggest having only one
>> getter and one setter for each property, i.e. not having a
>> setUnpackDestinationFile and setUnpackDestinationFileName, but only
>> having setUnpackDestination(File f).
>
>
> I'll agree it's not strict adherence to the JavaBean spec but
> following JavaBean conventions even partially can be useful. A few
> years ago I was treating some JavaMail objects like beans and between
> Java 1.3 and 1.4 the rules on how Java determined what was a legal
> javabean property got more strict. This caused problems when I did
> something similar to "message.bodyPart.subject" in a JSP el
> expression. and there was one getBodyPart() and two setBodyPart(Type1)
> and a setBodyPart(Type2).
>
>> As I see it, here is the list of areas I think the archiver interface
>> offers odd behaviour relative to  the JavaBean spec:
>>
>>   1)  The sourceFile property is recieved by getSourceFile(), but is set
>> by loadFile and loadFilebyPath.  Following the JavaBeans rules, this
>> means there is a readonly property called sourceFile, rather than this
>> being a property that is gettable and settable.
>>
>>     In addition to load* are bad names because they don't actually load
>> anything.
>>
>>   2)  The unpackDestination property has a getter, but no setter.
>>        The unpackDestinationFile property has a setter but no getter.
>>        The unpackDestinationName property has a setter but no getter
>
>
> Okay, I'll agree that unpackDestinationFile getter should be renamed
> to unpackDestination but the unpackDestinationName property should
> still keep a separate name and it's okay if it's a dynamic property
> that really sets the unpackDestination property.
>
>> >> 4)  None of your interfaces deal with the case of what to do if the
>> >> destination file is already there.  Choices are either defining it in
>> >> the interface, or adding a property defining what to do.  I would
>> >> suggest the latter, and would suggest that for Archiver this method
>> >> should take a FileFilter (since the unpack behaviour can be 
>> non-trivial)
>> >> and should default to FalseFileFilter.INSTANCE.  For the Compresser
>> >> interface a simple boolean is probably sufficient.  e.g.
>> >>
>> >>     Archiver.setOverwriteFilter(TrueFileFilter.INSTANCE)
>> >>     Compresser.setOverwrite(true);
>> >
>> >
>> > I'll disagree that this should be configurable. IMO it should just do
>> > whatever FileOutputStream or whatever is used does. It's the calling
>> > code's responsibility to handle any name collisions.
>> >
>> How can the calling code handle name collisions?  It takes significantly
>> more work for it to walk through all the entries in the archive and then
>> walking through all the files in the file ssytem.  Are you suggesting a
>> caller should only unpack into an empty directory?
>>
>> I'm not really sure I understand what your saying about
>> FileOutputStream.  The apis for the Archiver don't allow you to set an
>> OutputStream, and it's not really applicable to upack (which is writing
>> many files).
>
>
> I was confused about what you were referring to. I was thinking you
> wanted to specify the behavior for what would happen for creating a
> new archive on to an existing name.
>
> I'll agree the behavior for unpacking into an existing directory tree
> should be improved.
>
> -- 
> Sandy McArthur
>
> "He who dares not offend cannot be honest."
> - Thomas Paine
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>

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


Mime
View raw message