I'm not sure if there is some requirement I'm missing, but
1) It still seems strange to me that we have an interface that does two
things: pack an archive + unpack an archive, yet it has 16 methods on it.
Of these, 9 seem to be some variation of property getters/setters.
As far as I can tell, none of these properties are used by both the pack
and unpack process.
Methods that set/get properties only used
1) Why do we need getters and setters that take both a File and a
String as parameters? Why can't we standardize on just having a File
type for sourceFile and unpackDestination.
2) I'm still not sure why unpackDestination is a property and not just
a parameter on unpack? It seems that it is a property that is used in
one and only one place. It seems like logically its not a property of
the Archiver as much as a property of the actual packing process.
3) Why don't we have a constructor for each of the archivers that can
take a String or a File as the SourceFile (similar to how java.io.File,
java.util.zip.ZipFile, etc deal with this). If you didn't want to
enforce this as a convention for Archivers, it could easily be added
into the ArchiverType.getInstance() method. We could have ArchiverType
provide two getInstance methods, one could take a string and one could
take a file. This way the interface can be minimal, and we can hoist
type conversions off to helpers rather than the interface
4) What is the purpose of the ArchiverType? I'm assuming this is meant
to help people build their own factories? but the common case for
creating an Archiver is still to create using 'new'. Is this right?
--Will
Sandy McArthur wrote:
On 5/1/06, will pugh <[EMAIL PROTECTED]> wrote:
Sandy McArthur wrote:
> On 4/30/06, will pugh <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
