Maybe this was done to allow cases where an invoice has any tax items ? But
then the message is wrong, it is
Debug.logError(e, "Trouble getting InvoiceItem list", module);
should be
Debug.logError(e, "Trouble getting tax InvoiceItem list", module);
My 2 cents
Jacques
From: "Scott Gray" <[EMAIL PROTECTED]>
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.
