Thanks David, I guess it's pretty subjective and needs to be looked at
on a larger scale rather than just examining small chunks of code. I
might put that to one side for now unless I come across anything that
looks like it might obviously cause issues.
One example I came across that did concern me was in the
InvoiceWorker.getInvoiceTaxTotalBd() method where if it has trouble
pulling up the InvoiceItems it just logs an error and returns zero
tax. Any opinions (anyone) on whether it is a cause for concern and
if so what would be the correct approach? Throw the
GenericEntityException?
Thanks
Scott
2008/8/15 David E Jones <[EMAIL PROTECTED]>:
>
> As a generality it is important that whenever an exception is caught it is
> handled so that errors don't just "disappear...".
>
> However, there are different valid ways of handling an exception and
> sometimes it is best to just log an error or warning and move on, especially
> when the code is isolated such that passing it back to the caller doesn't
> make sense, or it doesn't affect program flow or anything else that might be
> done, but it is good to know what happened if anyone digs into a log anyway.
>
> One way or another the catch clause should:
>
> 1. throw another exception
> 2. return a message (for services and such)
> 3. add a message to the request or context (for events and such)
> 4. log the error and move on
>
> -David
>
>
> On Aug 14, 2008, at 3:08 AM, Scott Gray wrote:
>
>> Hi All
>>
>> While going through looking for the issue Adam mentioned, I'm seeing a
>> lot of the following:
>> List orl = null;
>> try {
>> orl = delegator.findByAnd(...);
>> } catch (GenericEntityException e) {
>> Debug.logError(e, module);
>> }
>> if (orl != null && orl.size() > 0) {
>> ...
>> }
>>
>> Shouldn't we always return an error if the delegator throws an
>> exception rather than continue processing? In the example above we're
>> treating an exception as being equivalent to an empty result which
>> seems a bit funny to me...
>>
>> Thanks
>> Scott
>>
>> 2008/8/13 Scott Gray <[EMAIL PROTECTED]>:
>>>
>>> Hi Adam,
>>>
>>> Did you have any plans to clean these up? I don't want to start if
>>> you're already on it.
>>>
>>> Thanks
>>> Scott
>>>
>>> 2008/8/13 Adam Heath <[EMAIL PROTECTED]>:
>>>>
>>>> As I've been going thru more ofbiz code, adding generics, I've
>>>> discovered a
>>>> bunch of the following pattern:
>>>>
>>>> ....
>>>> List items = delegator.findByAnd(...)
>>>> Iterator itemIt = UtilMisc.toIterator(items)
>>>> if (itemIt != null && itemIt.hasNext()) {
>>>> ....
>>>>
>>>> First off, delegator.findByAnd(and findList as well) never return null.
>>>> Ever. They will return an empty list if nothing is found. It's only
>>>> findOne(and friends) that return null.
>>>>
>>>> Because items is never null, toIterator always returns an iterator. So
>>>> itemIt is never null.
>>>>
>>>
>
>
