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
