You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@sandglass-software.com> on 2012/07/18 09:33:03 UTC
UtilValidate And Revision 883549
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) {
...
}
-Adrian
Re: UtilValidate And Revision 883549
Posted by Scott Gray <sc...@hotwaxmedia.com>.
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" <ad...@sandglass-software.com>
>>> 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
Re: UtilValidate And Revision 883549
Posted by Adrian Crum <ad...@sandglass-software.com>.
On 7/18/2012 9:02 AM, Jacques Le Roux wrote:
> From: "Adrian Crum" <ad...@sandglass-software.com>
>> 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
Re: UtilValidate And Revision 883549
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Adrian Crum" <ad...@sandglass-software.com>
>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...
Jacques
> -Adrian
>