Mark and all,

Thanks for making that fix.

I do have a bit of a nit to pick, though. Though line 256 (pre-patch
numbering) DOES throw an exception, the read method did NOT throw an
exception since it was caught internally; you can prove this to yourself by
removing the "throws" clause from the method signature (again, using the
code prior to the patch, of course) and see that it compiles without error.
Since IOException and its subclasses are checked exceptions, the method
would need the "throws" clause if it actually threw the exception. Indeed,
the patched version now does require the "throws" clause for IOException.

The reason I quibble about this (besides having mild OCD) is that there
actually are other places in the MASON code where exceptions are not
propagated up to the caller. Some of those are, I think, probably for the
best, but I think that some should be re-examined and similarly updated. I
understand that some people with legacy code may be relying on the engine to
continue running when certain exceptions are thrown, but I think it would be
better practice to make the programmer explicitly catch an exception and
consciously decide if s/he wants the program to continue or bail out. The
primary difficulty for legacy code will be for people who need their
simulations to run with underlying exceptions firing off -- I can't think of
any case where I haven't wanted to fix the problem causing the exception,
but there may be some, such as "if the configuration file wasn't found, I
want the simulation to continue with default parameters." There may also be
cases where checked exceptions were being hidden, which will require legacy
code to be patched with try/catch blocks around such calls, but I'm hoping
there aren't actually any cases like that lurking about. If there are, such
cases can be examined for the usefulness of suppressing the exception.
Alternatively, the checked exception could be suppressed while still
maintaining the principle of propagation by throwing an unchecked exception
instead of the original exception (not ideal, but perhaps a compromise, if
needed).

Best,
Matt L. Miller
UCDavis

On Wed, 13 Mar 2013 17:48:48 -0400, Mark Coletti <[log in to unmask]> wrote:

>On Wed, Mar 6, 2013 at 6:30 PM, Matt L. Miller <[log in to unmask]> wrote:
>
>> While I was doing some failure-mode testing on some code that uses
>> GeoMASON,
>> I have noticed that ShapeFileImporter.read(URL, GeomVectorField, Bag,
>> Class)
>> has a problem with its exception handling.
>>
>> The method declares that it throws a FileNotFoundException; however, it
>> actually never does so -- the entire IO section is surrounded in a try ...
>>
>
>Actually, it does throw a FileNotFoundException:
>
>    250         try
>    251         {
>    252             FileInputStream shpFileInputStream = new
>FileInputStream(shpFile.getFile());
>    253
>    254             if (shpFileInputStream == null)
>    255             {
>    256                 throw new FileNotFoundException(shpFile.getFile());
>    257             }
>
>
>
>> catch that catches all IOExceptions and prints an error message, but does
>> NOT propagate the exception back up to the caller. Thus, you get an error
>> message on STDERR, but then your code continues blithely along.
>>
>> I would recommend either adding a
>>
>> throw e;
>>
>>
>You're right.  It's bad form to catch exceptions without propagating them.
> I've committed changes whereby the exceptions are rethrown with
>appropriate changes to the function signatures.
>