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...@hlmksw.com> on 2010/02/02 20:27:38 UTC
FlexibleStringExpander (was: Form Widget labels disappearing)
In August 2008, FlexibleStringExpander class was redesigned to help
conserve memory. The main change needed to use the redesigned class was
to replace
FlexibleStringExpander fse = new FlexibleStringExpander("someExpression");
with
FlexibleStringExpander fse =
FlexibleStringExpander.getInstance("someExpression");
The getInstance method was designed to ALWAYS return an instance, even
when the method was called with a null or empty argument. Doing things
this way makes coding simpler - just treat the instance as if it
contains something. There is no need to check for null or empty expressions.
A simple project-wide search-and-replace was done to use the redesigned
class. The problem is, doing that left behind a lot of unnecessary code
that was needed for the old design.
It would be nice if we could get some of that legacy code cleaned up.
Here are a couple of suggested changes...
Replace
FlexibleStringExpander fse =
FlexibleStringExpander.getInstance("someExpression");
if (fse != null) {
// some code
}
with
FlexibleStringExpander fse =
FlexibleStringExpander.getInstance("someExpression");
// some code (getInstance never returns null)
Replace
FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var);
if (fse.getOriginal() != null) {
// some code
}
with
FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var);
if (!fse.isEmpty()) {
// some code
}
-Adrian
Jacques Le Roux wrote:
> Then we should have this kind of advices in the API or the documentation
> somewhere. We can't expect that future devs will pick this
> message easily. It would be easier for us to refer to somewhere as well.
>
> My 2 cts
>
> Jacques
>
> From: "Adrian Crum" <ad...@hlmksw.com>
>> I think you're reading too much into what I said.
>>
>> I didn't assign blame to anyone else - I'm using this as an
>> opportunity to instruct. I haven't forgotten the history of the code.
>> A lot of those unnecessary checks are left over from the older version
>> of FSE that used the new operator. So, for the benefit of
>> the other developers I'm explaining why that code is no longer necessary.
>>
>> And I'm not surprised by framework changes causing unintended side
>> effects. I usually time my framework commits so I have time
>> available to fix any problems that pop up.
>>
>> -Adrian
>>
>> David E Jones wrote:
>>> And that matters because... ?
>>>
>>> The problem is that whenever you change the framework there may be
>>> unintended side-effects because the way you think it should be
>>> used may not be the way it is being used (or even the way it was
>>> originally intended to be used).
>>>
>>> Even if you test a lot there is always the risk of this. So, the
>>> point is that you shouldn't be surprised by things like this.
>>>
>>> Yes, it is disappointing that people fail to use obvious things like
>>> an isEmpty method. Whatever the case, if you change how the
>>> code operates you ALWAYS risk these sorts of issues. It's still to
>>> say the problem was caused by how the class was being used...
>>> maybe convenient, but not really correct or useful. Yes, if only
>>> everyone would always develop things the way that I think they
>>> should be developed! Well, if that were the case for me, OFBiz would
>>> be a lot different right now... :)
>>>
>>> -David
>>>
>>>
>>> On Feb 2, 2010, at 11:28 AM, Adrian Crum wrote:
>>>
>>>> I looked through some example uses of FlexibleStringExpander and
>>>> there is a lot of inconsistency. Some client code expects
>>>> getOriginal() to always return a String - it doesn't check for null.
>>>> Other code checks for null.
>>>>
>>>> The whole point of the null instance was to eliminate all of the
>>>> checking for this or that. Just use it as if it contains
>>>> something. If it's empty it will do nothing.
>>>>
>>>> -Adrian
>>>>
>>>> David E Jones wrote:
>>>>> There's the fun of changing the framework...
>>>>> -David
>>>>> On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote:
>>>>>> Okay, I found the problem and committed a fix. We really need to
>>>>>> go through the framework and fix the code that uses
>>>>>> FlexibleStringExpander in inappropriate ways.
>>>>>>
>>>>>> FlexibleStringExpander.getInstance will ALWAYS return an instance,
>>>>>> so there is no need to check if an instance is equal to
>>>>>> null.
>>>>>>
>>>>>> Checking FlexibleStringExpander.getOriginal() for null or empty is
>>>>>> bad coding style. There is an isEmpty() method for that.
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>> Adrian Crum wrote:
>>>>>>> Hans,
>>>>>>> How is that commit affecting UI labels?
>>>>>>> -Adrian
>>>>>>> Hans Bakker wrote:
>>>>>>>> Hi this seems to be caused by r904592
>>>>>>>>
>>>>>>>> to revert this change:
>>>>>>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk
>>>>>>>> -r904592:904591
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Hans
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote:
>>>>>>>>> After taking an update today, a lot of form widget field labels
>>>>>>>>> seem to be missing from various screens. If any committers
>>>>>>>>> are aware of any changes they may have made which could have
>>>>>>>>> impacted this please take a look and see if it's related to
>>>>>>>>> your work.
>>>>>>>>>
>>>>>>>>> Here's an example:
>>>>>>>>> https://localhost:8443/catalog/control/FindProductConfigItems
>>>>>>>>> (only one of the 3 search option fields
>>>>>>>>> are presenting a field label)
>>>>>>>>> Here's another:
>>>>>>>>> https://localhost:8443/accounting/control/findPayments
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Scott
>>>>>>>>>
>>>>>>>>> HotWax Media
>>>>>>>>> http://www.hotwaxmedia.com
>>>>>>>>>
>>>>>>>>>
>>>
>>>
>>
>
>
Re: FlexibleStringExpander
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> FlexibleStringExpander fse =
>>>> FlexibleStringExpander.getInstance("someExpression");
>>>> if (fse != null) {
>>>> // some code
>>>> }
>>> This was always broken. Type foo = new Type(); foo will *never* be
>>> null, period. There are existing places in the code where this new
>>> Type() compared to null takes place, unrelated to FSE.
>> I was trying to provide a simplified example. You are correct - the
>> simplified example would never work to begin with. What the simplified
>> example might really look like:
>>
>> protected FlexibleStringExpander description = null;
>
> Just for reference, the correct fix here would remove the null default
> value, make it final, and remove the condition check below.
Exactly.
Re: FlexibleStringExpander
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> Adrian Crum wrote:
>>> FlexibleStringExpander fse =
>>> FlexibleStringExpander.getInstance("someExpression");
>>> if (fse != null) {
>>> // some code
>>> }
>>
>> This was always broken. Type foo = new Type(); foo will *never* be
>> null, period. There are existing places in the code where this new
>> Type() compared to null takes place, unrelated to FSE.
>
> I was trying to provide a simplified example. You are correct - the
> simplified example would never work to begin with. What the simplified
> example might really look like:
>
> protected FlexibleStringExpander description = null;
Just for reference, the correct fix here would remove the null default
value, make it final, and remove the condition check below.
> ...
> public MyClass(String expression) {
> if (expression != null) {
> description = FlexibleStringExpander.getInstance(expression);
> }
> }
> ...
> public String getDescription(Map context) {
> if (description != null) {
> return description.expandString(context);
> }
> ...
> }
>
> -Adrian
>
Re: FlexibleStringExpander
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> FlexibleStringExpander fse =
>> FlexibleStringExpander.getInstance("someExpression");
>> if (fse != null) {
>> // some code
>> }
>
> This was always broken. Type foo = new Type(); foo will *never* be
> null, period. There are existing places in the code where this new
> Type() compared to null takes place, unrelated to FSE.
I was trying to provide a simplified example. You are correct - the
simplified example would never work to begin with. What the simplified
example might really look like:
protected FlexibleStringExpander description = null;
...
public MyClass(String expression) {
if (expression != null) {
description = FlexibleStringExpander.getInstance(expression);
}
}
...
public String getDescription(Map context) {
if (description != null) {
return description.expandString(context);
}
...
}
-Adrian
Re: FlexibleStringExpander
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> FlexibleStringExpander fse =
> FlexibleStringExpander.getInstance("someExpression");
> if (fse != null) {
> // some code
> }
This was always broken. Type foo = new Type(); foo will *never* be
null, period. There are existing places in the code where this new
Type() compared to null takes place, unrelated to FSE.
Re: FlexibleStringExpander (was: Form Widget labels disappearing)
Posted by Jacques Le Roux <ja...@free.fr>.
Thanks Adrian,
When I will get a chance I will consider to document this. I guess at least in the API and I hope to begin a section for stuff like
that in the wiki.
Jacques
From: "Adrian Crum" <ad...@hlmksw.com>
> In August 2008, FlexibleStringExpander class was redesigned to help conserve memory. The main change needed to use the redesigned
> class was to replace
>
> FlexibleStringExpander fse = new FlexibleStringExpander("someExpression");
>
> with
>
> FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression");
>
> The getInstance method was designed to ALWAYS return an instance, even when the method was called with a null or empty argument.
> Doing things this way makes coding simpler - just treat the instance as if it contains something. There is no need to check for
> null or empty expressions.
>
> A simple project-wide search-and-replace was done to use the redesigned class. The problem is, doing that left behind a lot of
> unnecessary code that was needed for the old design.
>
> It would be nice if we could get some of that legacy code cleaned up. Here are a couple of suggested changes...
>
> Replace
>
> FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression");
> if (fse != null) {
> // some code
> }
>
> with
>
> FlexibleStringExpander fse = FlexibleStringExpander.getInstance("someExpression");
> // some code (getInstance never returns null)
>
> Replace
>
> FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var);
> if (fse.getOriginal() != null) {
> // some code
> }
>
> with
>
> FlexibleStringExpander fse = FlexibleStringExpander.getInstance(var);
> if (!fse.isEmpty()) {
> // some code
> }
>
> -Adrian
>
>
> Jacques Le Roux wrote:
>> Then we should have this kind of advices in the API or the documentation somewhere. We can't expect that future devs will pick
>> this
>> message easily. It would be easier for us to refer to somewhere as well.
>>
>> My 2 cts
>>
>> Jacques
>>
>> From: "Adrian Crum" <ad...@hlmksw.com>
>>> I think you're reading too much into what I said.
>>>
>>> I didn't assign blame to anyone else - I'm using this as an opportunity to instruct. I haven't forgotten the history of the
>>> code.
>>> A lot of those unnecessary checks are left over from the older version of FSE that used the new operator. So, for the benefit of
>>> the other developers I'm explaining why that code is no longer necessary.
>>>
>>> And I'm not surprised by framework changes causing unintended side effects. I usually time my framework commits so I have time
>>> available to fix any problems that pop up.
>>>
>>> -Adrian
>>>
>>> David E Jones wrote:
>>>> And that matters because... ?
>>>>
>>>> The problem is that whenever you change the framework there may be unintended side-effects because the way you think it should
>>>> be
>>>> used may not be the way it is being used (or even the way it was originally intended to be used).
>>>>
>>>> Even if you test a lot there is always the risk of this. So, the point is that you shouldn't be surprised by things like this.
>>>>
>>>> Yes, it is disappointing that people fail to use obvious things like an isEmpty method. Whatever the case, if you change how
>>>> the
>>>> code operates you ALWAYS risk these sorts of issues. It's still to say the problem was caused by how the class was being
>>>> used...
>>>> maybe convenient, but not really correct or useful. Yes, if only everyone would always develop things the way that I think they
>>>> should be developed! Well, if that were the case for me, OFBiz would be a lot different right now... :)
>>>>
>>>> -David
>>>>
>>>>
>>>> On Feb 2, 2010, at 11:28 AM, Adrian Crum wrote:
>>>>
>>>>> I looked through some example uses of FlexibleStringExpander and there is a lot of inconsistency. Some client code expects
>>>>> getOriginal() to always return a String - it doesn't check for null. Other code checks for null.
>>>>>
>>>>> The whole point of the null instance was to eliminate all of the checking for this or that. Just use it as if it contains
>>>>> something. If it's empty it will do nothing.
>>>>>
>>>>> -Adrian
>>>>>
>>>>> David E Jones wrote:
>>>>>> There's the fun of changing the framework...
>>>>>> -David
>>>>>> On Feb 2, 2010, at 10:40 AM, Adrian Crum wrote:
>>>>>>> Okay, I found the problem and committed a fix. We really need to go through the framework and fix the code that uses
>>>>>>> FlexibleStringExpander in inappropriate ways.
>>>>>>>
>>>>>>> FlexibleStringExpander.getInstance will ALWAYS return an instance, so there is no need to check if an instance is equal to
>>>>>>> null.
>>>>>>>
>>>>>>> Checking FlexibleStringExpander.getOriginal() for null or empty is bad coding style. There is an isEmpty() method for that.
>>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>> Adrian Crum wrote:
>>>>>>>> Hans,
>>>>>>>> How is that commit affecting UI labels?
>>>>>>>> -Adrian
>>>>>>>> Hans Bakker wrote:
>>>>>>>>> Hi this seems to be caused by r904592
>>>>>>>>>
>>>>>>>>> to revert this change:
>>>>>>>>> svn merge http://svn.apache.org/repos/asf/ofbiz/trunk -r904592:904591
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Hans
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, 2010-02-01 at 23:07 -0800, Scott Gray wrote:
>>>>>>>>>> After taking an update today, a lot of form widget field labels seem to be missing from various screens. If any
>>>>>>>>>> committers
>>>>>>>>>> are aware of any changes they may have made which could have impacted this please take a look and see if it's related to
>>>>>>>>>> your work.
>>>>>>>>>>
>>>>>>>>>> Here's an example: https://localhost:8443/catalog/control/FindProductConfigItems (only one of the 3 search option fields
>>>>>>>>>> are presenting a field label)
>>>>>>>>>> Here's another: https://localhost:8443/accounting/control/findPayments
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Scott
>>>>>>>>>>
>>>>>>>>>> HotWax Media
>>>>>>>>>> http://www.hotwaxmedia.com
>>>>>>>>>>
>>>>>>>>>>
>>>>
>>>>
>>>
>>
>>
>