On 18/07/2012, at 8:37 PM, Adrian Crum wrote: > On 7/18/2012 9:02 AM, Jacques Le Roux wrote: >> From: "Adrian Crum" <[email protected]> >>> I keep finding unnecessary calls to UtilValidate.isEmpty in the project. It >>> seems they were added in a global S&R and committed in >>> revision 883549. >>> >>> I believe this is a bad pattern to use. If I want to call a method on >>> object A, why would I call a different method on object B >>> that calls the method on object A? I can just call object A's method >>> directly and avoid using object (or class) B. Not only is >>> this indirection unnecessary, it is also confusing. >>> >>> From my perspective, there are serious drawbacks to using this approach: >>> >>> 1. Nearly every class is made dependent on UtilValidate. >>> 2. It hurts performance. Static method calls are not free. >>> >>> There are places where UtilValidate.isEmpty is used when it is completely >>> inappropriate - like for checking DOM Element >>> attributes. They are never null, and java.lang.String has an isEmpty() >>> method. There are other places where it is used on objects >>> that are never null. >>> >>> UtilValidate.isEmpty should be used when you don't know the object's type >>> or if it is null: >>> >>> Object mysteryObj = methodThatMightReturnNull(); >>> if (UtilValidate.isEmpty(mysteryObj) { >>> ... >>> } >> >> As I commented at >> http://svn.apache.org/viewvc?view=revision&revision=883549, what I did was >> to "Use UtilValidate.isNotEmpty methods instead of (obj != null) || >> (obj.length > 0) and (obj != null) || (obj.size > 0)" >> IRRW I used a simple regexp and did a complete review so no other cases >> should have slipped. I will do another complete review of that commit and >> will fix necessary hunks if any. >> But maybe the issue was upstream and I simply replaced cases where >> (obj.size > 0) or (obj.length > 0) already did not make sense. >> >> Also Paul Foxworthy began a related work "Possible runtime errors with >> UtilValidate.isEmpty(Object) should be rather caught during >> compilation" https://issues.apache.org/jira/browse/OFBIZ-4427. I simply did >> not get time to review it yet... > > I am sure that the commit was not responsible for all occurrences of the > pattern, and I apologize for making it sound that way. I don't think we need > to do another global S&R to replace it. I only want to encourage everyone to > think about the method's purpose and use it only when necessary. > > -Adrian
A massive +1, instanceof checks are expensive so I don't like the isEmpty(Object) method at all. I think in a previous discussion we talked about moving that method elsewhere so minilang could still use it while attempting to hide it's visibility to java/groovy devs. Regards Scott
