findOne signatures refactoring

[Jacques Le Roux]

Hi,

I suggest to refactor findOne signatures. Or actually only the signature w/out ellipsis. When we introduced the signature with
elilpsis the cache argument was inevitably in the middle. We kept the signature for the other method. I was caught recently by
changing a method call from ellipsis signature to the other. I introduced UtilMisc.toMap for the last argument because I had 2
values and changed for one only, hence ellipsis could not be used. I did that in a hurry, not thinking about the argument swap.

So it would be only a matter of swapping the cache argument from last place to second in the old signature. IMO it makes sense to
have "consistent" signatures... Maybe findOne is not the only one to change...

Opinions?

Jacques

[Martin Kreidenweis]

Hi,

> I suggest to refactor findOne signatures. Or actually only the signature w/out ellipsis. When we
> introduced the signature with elilpsis the cache argument was inevitably in the middle. We kept the
> signature for the other method. I was caught recently by changing a method call from ellipsis
> signature to the other. I introduced UtilMisc.toMap for the last argument because I had 2 values and
> changed for one only, hence ellipsis could not be used. I did that in a hurry, not thinking about
> the argument swap.
>
> So it would be only a matter of swapping the cache argument from last place to second in the old
> signature. IMO it makes sense to have "consistent" signatures... Maybe findOne is not the only one
> to change...

Delegator.findList() also has the useCache param as the last one. So this is consistent with
findOne. The doCacheClear params also are always last.

The findOne with ellipsis is the only exception. I think that's OK.

As we write almost all our code in Groovy now, the findOne with ellipsis is not really necessary any
more for us. Groovy has a nice map literal syntax.

Martin

[Scott Gray-2]

No matter how many times I read what you've written below I can't understand what issue you ran into?

You had:
delegator.findOne("Entity", true, "field1", value1);
and you tried to change it to:
delegator.findOne("Entity", true, UtilMisc.toMap("field1", value1));
?

That should still work since findOne(String, boolean, Object...) would still be used and it calls UtilMisc.toMap(Object...) which supports exactly this scenario.  What is the problem?

Regards
Scott

On 27/09/2011, at 8:24 PM, Jacques Le Roux wrote:

> Hi,
>
> I suggest to refactor findOne signatures. Or actually only the signature w/out ellipsis. When we introduced the signature with elilpsis the cache argument was inevitably in the middle. We kept the signature for the other method. I was caught recently by changing a method call from ellipsis signature to the other. I introduced UtilMisc.toMap for the last argument because I had 2 values and changed for one only, hence ellipsis could not be used. I did that in a hurry, not thinking about the argument swap.
>
> So it would be only a matter of swapping the cache argument from last place to second in the old signature. IMO it makes sense to have "consistent" signatures... Maybe findOne is not the only one to change...
>
> Opinions?
>
> Jacques

[Jacques Le Roux]

I had 2 values

>> changing a method call from ellipsis signature to the other. I introduced UtilMisc.toMap for the last argument because I had 2
>> values and changed for one only, hence ellipsis could not be used. I did that in a hurry, not thinking about the argument swap.

Let me try to explain from your example:

delegator.findOne("Entity", true, "field1", value1, "field2", value2);
and tried to change it to:
delegator.findOne("Entity", true, UtilMisc.toMap("field1", value1));

As you can see the 2d line is taken by the compiles has being using the 1st signature. So it compiles, but is wrong.
The last cons at http://www.codeproject.com/KB/java/Java5FeaturesII.aspx#pre10 explains the reason
<<If you are not careful, you may have trouble with method overloading as the varargs may increase the chance of parameter collision
when methods are overloaded.>>

Jacques



Scott Gray wrote:

> No matter how many times I read what you've written below I can't understand what issue you ran into?
>
> You had:
> delegator.findOne("Entity", true, "field1", value1);
> and you tried to change it to:
> delegator.findOne("Entity", true, UtilMisc.toMap("field1", value1));
> ?
>
> That should still work since findOne(String, boolean, Object...) would still be used and it calls UtilMisc.toMap(Object...) which
> supports exactly this scenario.  What is the problem?
>
> Regards
> Scott
>
> On 27/09/2011, at 8:24 PM, Jacques Le Roux wrote:
>
>> Hi,
>>
>> I suggest to refactor findOne signatures. Or actually only the signature w/out ellipsis. When we introduced the signature with
>> elilpsis the cache argument was inevitably in the middle. We kept the signature for the other method. I was caught recently by
>> changing a method call from ellipsis signature to the other. I introduced UtilMisc.toMap for the last argument because I had 2
>> values and changed for one only, hence ellipsis could not be used. I did that in a hurry, not thinking about the argument swap.
>>
>> So it would be only a matter of swapping the cache argument from last place to second in the old signature. IMO it makes sense
>> to have "consistent" signatures... Maybe findOne is not the only one to change...
>>
>> Opinions?
>>
>> Jacques

[Martin Kreidenweis]

Hi,

>>> changing a method call from ellipsis signature to the other. I introduced UtilMisc.toMap for the
>>> last argument because I had 2
>>> values and changed for one only, hence ellipsis could not be used. I did that in a hurry, not
>>> thinking about the argument swap.
>
> Let me try to explain from your example:
>
> delegator.findOne("Entity", true, "field1", value1, "field2", value2);
> and tried to change it to:
> delegator.findOne("Entity", true, UtilMisc.toMap("field1", value1));

Now that Scott has told us that UtilMisc.toMap() supports the case where there is only one param and
it already is a map, I don't see any reason to change the signature.

I just tried it out. All three those lines found the right entity for me:

delegator.findOne("Product", true, "productId", testProductId);
delegator.findOne("Product", UtilMisc.toMap("productId", testProductId), true);
delegator.findOne("Product", true, UtilMisc.toMap("productId", testProductId));

I'm against changing the signature. I like the decreasing importance structure of the parameters
(what-which-how/entityname-conditions-useCache) the find* methods have. And we would have quite some
code to migrate.

BTW, we usually would write the statement in Groovy like that:

delegator.findOne("Product", [productId: testProductId], true)

We changed the ant tasks so we can use Groovy also for the compiled code -- service and event
classes -- not only for action scripts. Maybe this is something the OFBiz project wants to consider.
I think it makes most of the code easier to write and read.
A disadvantage is, that the compile times are significantly higher. But it isn't often that one
needs to do a clean build. :-)

Martin

[Jacques Le Roux]

Right, it was another (logical) issue in code. I mixed up both when it was actually obvious that a map was rendered by
UtilMisc.toMap("field1", value1) hence no issues :/

Thanks for your help (both Scott and you)

Jacques

From: "Martin Kreidenweis" <[hidden email]>

> Hi,
>
>>>> changing a method call from ellipsis signature to the other. I introduced UtilMisc.toMap for the
>>>> last argument because I had 2
>>>> values and changed for one only, hence ellipsis could not be used. I did that in a hurry, not
>>>> thinking about the argument swap.
>>
>> Let me try to explain from your example:
>>
>> delegator.findOne("Entity", true, "field1", value1, "field2", value2);
>> and tried to change it to:
>> delegator.findOne("Entity", true, UtilMisc.toMap("field1", value1));
>
> Now that Scott has told us that UtilMisc.toMap() supports the case where there is only one param and
> it already is a map, I don't see any reason to change the signature.
>
> I just tried it out. All three those lines found the right entity for me:
>
> delegator.findOne("Product", true, "productId", testProductId);
> delegator.findOne("Product", UtilMisc.toMap("productId", testProductId), true);
> delegator.findOne("Product", true, UtilMisc.toMap("productId", testProductId));
>
> I'm against changing the signature. I like the decreasing importance structure of the parameters
> (what-which-how/entityname-conditions-useCache) the find* methods have. And we would have quite some
> code to migrate.
>
> BTW, we usually would write the statement in Groovy like that:
>
> delegator.findOne("Product", [productId: testProductId], true)
>
> We changed the ant tasks so we can use Groovy also for the compiled code -- service and event
> classes -- not only for action scripts. Maybe this is something the OFBiz project wants to consider.
> I think it makes most of the code easier to write and read.
> A disadvantage is, that the compile times are significantly higher. But it isn't often that one
> needs to do a clean build. :-)
>
> Martin