commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel F. Savarese" <...@savarese.org>
Subject Re: [net] checked in parser factory implementation
Date Tue, 06 Jan 2004 18:07:55 GMT

In message <200401051207.29052.scohen@javactivity.org>, steve cohen writes:
>1) deprecate listFiles() methods that take a FileEntryParser parameter.
>2) reimplement listFiles() to do what getFileList(null) now does.
>3) reimplement listFiles(String pathname) to do what getFileList(null, 
>pathname) now does.
>4) rename getFileList(String parserKey, String pathname) to listFiles(String 
>parserKey, String pathname).  Even though autodetection will be the primary 
>use case, it already will not work for every system (see EnterpriseUnix) and 
>there must be some way around it.  This is that way.
>5) don't use properties to select the parser factory in our library.  Instead,
>add a property and a setter method.  Initialize in the library to use the 
>default.  Have I missed anything?

Yes, that's exactly what I was suggesting.  It also occurred to me later
that we may want to cache the system name in getSystemName (save it on
the first call, return it on subsequent calls, clear it on disconnect),
since it will be called more frequently now that you've added
autodetection.

>One problem remains.  getFileList() throws a new exception: 
>ParserInitializationException.  If we simply rename these methods to
>listFiles() we will break lots of code.  That was my primary reason for giving
> 
>them a different name.  One way around this would be to make 
>ParserInitializationException a RuntimeException.  That makes sense, as the 
>error is not recoverable and is always due to a programming error - passing 
>in a key that the system cannot parse, specifying a class that has not been 
>made available.  Would you agree?  

Another choice is to make it extend IOException, but that doesn't seem right.
I guess my preference would be to catch the exception in listFiles() and
rethrow an IOException that bubbles up the original exception.  The problem
with that is that standard support for wrapping exceptions is present only
in J2SE 1.4.  Also, what's the real difference between adding an
FTPFileListException (or whatever name) derived from IOException and
throwing that instead of just letting a ParserInitializationException
derived from RuntimeException pass through.  The reason I'm conflicted
is that it doesn't seem to me that the caller of listFiles should be
bothered to know about the internal implementation of the method, which
the throwing of ParserInitializationException requires.  If we make
ParserInitializationException a RuntimeException we can always go with
the catch and throw later.  Still, I'm wondering if FTPFileEntryParserFactory
should have a boolean isRecognized(String key) method or something similar
to avoid the expense of a try/catch block (i.e., you ensure a key is
supported before calling createFileEntryParser, and throw an
FTPSystemNotSupportedException or something).  I'm also wondering
a good number of other things (like how to avoid new FooEntryParser()
in createFileEntryParser on every call to listFiles), so I'll stop now
and say that I support your suggestion to make ParserInitializationException
a RuntimeException.  It doesn't preclude additional tweaks we may
think of before a final release.

>in Effective Java lays this out clearly enough.  If it was called 
>PreconditionViolationException or some such, it would be much clearer.  As it 
>is, I've seen too many instance of exceptions being made RuntimeExceptions 
>simply because the programmer thought that propagating it up though the call 
>stack was too difficult.  But I don't think that's the case here.  This is 
>legitimately a RuntimeException).  

I agree, but it depends on how you anticipate the method being used.  An
argument can be made for createFileEntryParser to not throw an exception
and instead return null.  I wouldn't advocate that here because we're
talking about a factory which creates new objects and not a mapper
that pairs keys with pre-existing objects.

>Agree here too, although the idea that FTPFileListParserImpl will no longer 
>implement FTPFileListParser is extremely odd.  I suggest we might want rename 
>this class now to FTPFileEntryParserImpl.  As it's abstract, I don't think 
>we'd be impacting anyone.

That sounds like the right thing to do.  So I guess we also deprecate
FTPFileListParserImpl?

daniel



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