Discovery of anti-pattern
Locked
 
|
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. |
|
|
could you put this in the best coding practices
http://docs.ofbiz.org/display/OFBADMIN/Best+Practices+Guide Adam Heath sent the following on 8/12/2008 8:04 AM: > 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. > > > |
Re: Discovery of anti-pattern
Administrator
|
I dont think this needs a best practice point. I guess it's something that have been copied/pasted from a 1st oversight..
We should only fix them and they should not reappear. Else we will end up with a lot of points like that... My 2 cents Jacques From: "BJ Freeman" <[hidden email]> > could you put this in the best coding practices > http://docs.ofbiz.org/display/OFBADMIN/Best+Practices+Guide > > Adam Heath sent the following on 8/12/2008 8:04 AM: >> 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. >> >> >> > |
|
|
In reply to this post by Adam Heath-2
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 <[hidden email]>: > 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. > |
|
|
Scott Gray wrote:
> Did you have any plans to clean these up? I don't want to start if > you're already on it. Not at the moment, no. |
|
|
In reply to this post by Scott Gray
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 <[hidden email]>: > 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 <[hidden email]>: >> 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. >> > |
|
|
I remember to fix similar bug, where catching GenericEntityException was
considered for empty result. I think this is not a good practice (any more). Bilgin |
Re: Discovery of anti-pattern
|
Administrator
|
In reply to this post by Scott Gray
We have also 322
if (* != null && *.size() > 0) { where * is a variable name (String or Collection). Those should really better rewritten if (UtilValidate.isNotEmpty(*)) { Jacques From: "Scott Gray" <[hidden email]> > 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 <[hidden email]>: >> 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 <[hidden email]>: >>> 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. >>> >> > |
|
|
In reply to this post by Scott Gray
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 <[hidden email]>: >> 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 <[hidden email]>: >>> 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. >>> >> |
|
|
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 <[hidden email]>: > > 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 <[hidden email]>: >>> >>> 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 <[hidden email]>: >>>> >>>> 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. >>>> >>> > > |
Re: Discovery of anti-pattern
|
Administrator
|
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" <[hidden email]> > 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 <[hidden email]>: >> >> 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 <[hidden email]>: >>>> >>>> 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 <[hidden email]>: >>>>> >>>>> 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. >>>>> >>>> >> >> > |
|
|
Hi Jacques
A lack of results won't cause the delegator to throw an exception, assuming the model entity and the query were sound the exception would come from a problem actually querying the database or table. Regards Scott 2008/8/17 Jacques Le Roux <[hidden email]>: > 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" <[hidden email]> >> >> 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 <[hidden email]>: >>> >>> 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 <[hidden email]>: >>>>> >>>>> 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 <[hidden email]>: >>>>>> >>>>>> 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. >>>>>> >>>>> >>> >>> >> > |
|
|
In reply to this post by Scott Gray
On Aug 15, 2008, at 6:00 AM, Scott Gray wrote: > 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? I'm not a big fan of the pattern used in the InvoiceWorker and OrderWorker files. For starters small worker methods like these should throw exceptions instead of trying to catch and hide them, or in other words they are not a good example of when to handle errors internally. That's aside from the fact that they represent a funny pattern for data preparation logic and might be better as a few bigger methods that do more instead of so many little tiny methods that are as tricky to keep track of as the data model fields and such themselves... -David |
Re: Discovery of anti-pattern
|
Administrator
|
In reply to this post by Scott Gray
Hi Scott,
True, so I can't see any reasonable reasons for this ! Jacques From: "Scott Gray" <[hidden email]> > Hi Jacques > > A lack of results won't cause the delegator to throw an exception, > assuming the model entity and the query were sound the exception would > come from a problem actually querying the database or table. > > Regards > Scott > > 2008/8/17 Jacques Le Roux <[hidden email]>: >> 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" <[hidden email]> >>> >>> 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 <[hidden email]>: >>>> >>>> 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 <[hidden email]>: >>>>>> >>>>>> 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 <[hidden email]>: >>>>>>> >>>>>>> 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. >>>>>>> >>>>>> >>>> >>>> >>> >> > |
Re: Discovery of anti-pattern
|
Administrator
|
In reply to this post by Jacques Le Roux
Fixed in revision: 686591
Jacques From: "Jacques Le Roux" <[hidden email]> > We have also 322 > if (* != null && *.size() > 0) { > where * is a variable name (String or Collection). Those should really better rewritten > if (UtilValidate.isNotEmpty(*)) { > > Jacques > > From: "Scott Gray" <[hidden email]> >> 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 <[hidden email]>: >>> 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 <[hidden email]>: >>>> 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. >>>> >>> >> > |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |