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