UtilValidate And Revision 883549

[Adrian Crum-3]

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

[Jacques Le Roux]

From: "Adrian Crum" <[hidden email]>

>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
>

[Adrian Crum-3]

On 7/18/2012 9:02 AM, Jacques Le Roux wrote:

> From: "Adrian Crum" <[hidden email]>
>> 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

[Paul Foxworthy]

Hi Adrian,

I agree there's an overuse of UtilValidate.isEmpty and UtilValidate.isNotEmpty.

I see no point at all in using these methods to test for nullity. == null or != null are perfectly fine.

I agree there's no point when you know the value is not null, and just want to test for emptiness. Just like you said, the DOM methods like getAttribute() are an example of that.

I see *some* point if the class supports the concept of emptiness and you aren't sure if a value is null or not. So with collections, arrays, strings and so on, if these methods are not used, you have to test for both nullity and emptiness. UtilValidate gives you one method call instead of two tests, and you don't have to work out whether you need the length field (arrays), the length() method (String and CharSequence), the size() method (collections), or isEmpty (from the org.ofbiz.base.lang.IsEmpty interface). A good JVM should inline the expression, so there might not be a performance price to pay.

Those reasons are not compelling, and I would not be be troubled if the methods disappeared altogether. I do think a good first step would be to remove uses of isEmpty and isNotEmpty which are only testing for nullity.

I did some work on this in a patch in OFBIZ-4427 which hasn't been committed. When the test was just for nullity, I eliminated use of UtilValidate. My patch deprecated UtilValidate.isEmpty(Object) and UtilValidate.isNotEmpty(Object), because I think these are pointless. While the patch does catch many calls to these methods, there are more. My hope was once the deprecation went in, several of us could work on the lines highlighted by the deprecation warning.

Cheers

Paul Foxworthy

Adrian Crum-3 wrote

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

[Scott Gray-2]

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" <[hidden email]>
>>> 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