You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by Curt Hagenlocher <cu...@motek.com> on 2000/09/20 01:52:51 UTC
Duplicate Code
I noticed that there's some code duplication between
org.apache.struts.util.PropertyUtils and org.apache.struts.util.BeanUtils.
Is this intentional? Is one of these more "right" than the other, if I
want to reuse this code in my own project?
--
Curt Hagenlocher
curth@motek.com
Re: Duplicate Code
Posted by "Craig R. McClanahan" <Cr...@eng.sun.com>.
See intermixed below.
Ate Douma wrote:
> Craig,
>
> Maybe you've already encountered them while refactoring, but I found two
> bugs
> in org.apache.struts.util.PropertyUtils both related to indexed properties:
> 1. In method Class getPropertyType( Object, String )
> the return value of the getPropertyType() method of a looked up
> PropertyDescriptor
> is returned.
> But... for an indexed property (thus: having a
> IndexedPropertyDescriptor) this method
> will return null if not a non indexed property accessor exists.
> A simple patch I've implemented is:
> <<< snip >>>
> if ( descriptor instanceof IndexedPropertyDescriptor )
> return
> (((IndexedPropertyDescriptor)descriptor).getIndexedPropertyType());
> else
> return (descriptor.getPropertyType());
> <<< end snip >>>
> 2. In method void setIndexedProperty(Object, String, int, Object)
> first an attempt is made to invoke the indexedWriteMethod of an indexed
> property.
> But... after that succeeds, a return statement is missing, and instead
> the method
> tries to update the indexed property by first retieving its Array and
> then setting the
> indexed element within.
> This works if an Array readMethod is defined (although setting the value
> is done twice then)
> but results in an exception if its not.
> Simple fix: add an return statement after the invokation of the
> writeMethod.
>
I have patched PropertyUtils as you described. Thanks!
>
> Besides the above bugs in PropertyUtils I've encountered some problems with
> the BeanUtils class.
>
> The populate method will try to convert the request parameters into the
> correct (class) type of the bean property.
> When an empty request parameter value is submitted, the convert<Class>
> methods will always return a default initialized
> <Class> instance, e.g. Long(0), Float(0.0) etc.
> This is fine, and even required for bean properties of native java
> datatypes, but for class wrapper types this is not really
> needed and produced in our application a lot of unexplainable errors at
> first.
> When an user clears an input item on a html form we prefer to receive this
> information as a null value, as a 0 (zero) or false value
> could have another meaning altogether in certain situations (and to support
> this all our ActionForm bean properties are never of native
> datatype; if needed, at least not the setter methods which can handle null
> parameters appropriately).
>
> Thus, I've made an major change in all of the convert<Class> methods:
> 1. I've added an additional parameter boolean nullAllowed:
> If the specified value is empty and nullAllowed, return null.
> If the specified value is not empty but an NumberFormatException is
> encountered, return null.
> Otherwise, the original convertion logic is used.
> 2. In all calls to the convert<Class> methods I've specified the appropriate
> boolean parameter depending on the property type.
>
> If the reasoning for doing so makes any sense, do you think these changes
> could be encorporated in later versions of struts as we really depend on
> them.
> I know this can break code that already depends on the default value
> initialization for Class type properties, but maybe an configuration
> parameter can
> be given which enables this type of behavior conditionally.
>
I can see the reasoning that led you to this approach. A way to implement it
that doesn't break backwards compatibility would be to have two versions of each
convert method -- one with and one without the nullAllowed parameter. That way,
you could have either kind of semantics. Does that make sense?
>
> Finally, I've changed the populate method to make use of the PropertyUtils
> class methods as we needed the nested and indexed properties in our
> ActionForms.
> This was quite a hack, but right now it works great and we are able to
> modify complex spreadsheet/matrix tables with reconfigurable row definitions
> etc. all through
> quite simple form definitions and generic dynamic matrix ActionForm beans.
> If you whould be interested in this (a little bit clumsy but effective)
> hack, please let me know.
>
Interesting timing ... finishing up BeanUtils was on my list of stuff to do this
week. If you wanted to send me your version of BeanUtils in private mail, I
could try to integrate it in a way compatible with what you are doing with it.
Also, if you cared to create one, I think lots of people would be interested in
a brief description of how you deal with spreadsheet/matrix type tables.
>
> Ate Douma
> iWise BV
> ate.douma@iwise.nl
>
Craig
====================
See you at ApacheCon Europe <http://www.apachecon.com>!
Session VS01 (23-Oct 13h00-17h00): Sun Technical Briefing
Session T06 (24-Oct 14h00-15h00): Migrating Apache JServ
Applications to Tomcat
Re: Duplicate Code
Posted by Ate Douma <at...@iwise1.demon.nl>.
Craig,
Maybe you've already encountered them while refactoring, but I found two
bugs
in org.apache.struts.util.PropertyUtils both related to indexed properties:
1. In method Class getPropertyType( Object, String )
the return value of the getPropertyType() method of a looked up
PropertyDescriptor
is returned.
But... for an indexed property (thus: having a
IndexedPropertyDescriptor) this method
will return null if not a non indexed property accessor exists.
A simple patch I've implemented is:
<<< snip >>>
if ( descriptor instanceof IndexedPropertyDescriptor )
return
(((IndexedPropertyDescriptor)descriptor).getIndexedPropertyType());
else
return (descriptor.getPropertyType());
<<< end snip >>>
2. In method void setIndexedProperty(Object, String, int, Object)
first an attempt is made to invoke the indexedWriteMethod of an indexed
property.
But... after that succeeds, a return statement is missing, and instead
the method
tries to update the indexed property by first retieving its Array and
then setting the
indexed element within.
This works if an Array readMethod is defined (although setting the value
is done twice then)
but results in an exception if its not.
Simple fix: add an return statement after the invokation of the
writeMethod.
Besides the above bugs in PropertyUtils I've encountered some problems with
the BeanUtils class.
The populate method will try to convert the request parameters into the
correct (class) type of the bean property.
When an empty request parameter value is submitted, the convert<Class>
methods will always return a default initialized
<Class> instance, e.g. Long(0), Float(0.0) etc.
This is fine, and even required for bean properties of native java
datatypes, but for class wrapper types this is not really
needed and produced in our application a lot of unexplainable errors at
first.
When an user clears an input item on a html form we prefer to receive this
information as a null value, as a 0 (zero) or false value
could have another meaning altogether in certain situations (and to support
this all our ActionForm bean properties are never of native
datatype; if needed, at least not the setter methods which can handle null
parameters appropriately).
Thus, I've made an major change in all of the convert<Class> methods:
1. I've added an additional parameter boolean nullAllowed:
If the specified value is empty and nullAllowed, return null.
If the specified value is not empty but an NumberFormatException is
encountered, return null.
Otherwise, the original convertion logic is used.
2. In all calls to the convert<Class> methods I've specified the appropriate
boolean parameter depending on the property type.
If the reasoning for doing so makes any sense, do you think these changes
could be encorporated in later versions of struts as we really depend on
them.
I know this can break code that already depends on the default value
initialization for Class type properties, but maybe an configuration
parameter can
be given which enables this type of behavior conditionally.
Finally, I've changed the populate method to make use of the PropertyUtils
class methods as we needed the nested and indexed properties in our
ActionForms.
This was quite a hack, but right now it works great and we are able to
modify complex spreadsheet/matrix tables with reconfigurable row definitions
etc. all through
quite simple form definitions and generic dynamic matrix ActionForm beans.
If you whould be interested in this (a little bit clumsy but effective)
hack, please let me know.
Ate Douma
iWise BV
ate.douma@iwise.nl
----- Original Message -----
From: "Craig R. McClanahan" <Cr...@eng.sun.com>
To: <st...@jakarta.apache.org>
Sent: Wednesday, September 20, 2000 2:41 AM
Subject: Re: Duplicate Code
> Curt Hagenlocher wrote:
>
> > I noticed that there's some code duplication between
> > org.apache.struts.util.PropertyUtils and
org.apache.struts.util.BeanUtils.
> > Is this intentional? Is one of these more "right" than the other, if I
> > want to reuse this code in my own project?
> >
>
> Sharp eyes!
>
> There is some redundancy at the moment because I am part way through a
> separation of the code that deals strictly with Java reflection
> (PropertyUtils) and the code that will deal with conversion to and from
> request parameters (BeanUtils). At the moment, PropertyUtils is looking
> pretty solid -- BeanUtils will shortly be shedding the methods that are
> duplications, which should be completed in time for the 1.0 release I'd
like
> to have by ApacheCon Europe (Oct 23-25).
>
> If there are particular methods in BeanUtils you are planning to use let
me
> know and I can warn of potential problems.
>
> >
> > --
> > Curt Hagenlocher
> > curth@motek.com
>
> Craig
>
> --
> ====================
> See you at ApacheCon Europe <http://www.apachecon.com>!
> Session VS01 (23-Oct 13h00-17h00): Sun Technical Briefing
> Session T06 (24-Oct 14h00-15h00): Migrating Apache JServ
> Applications to Tomcat
>
>
>
Re: Duplicate Code
Posted by "Craig R. McClanahan" <Cr...@eng.sun.com>.
Curt Hagenlocher wrote:
> I noticed that there's some code duplication between
> org.apache.struts.util.PropertyUtils and org.apache.struts.util.BeanUtils.
> Is this intentional? Is one of these more "right" than the other, if I
> want to reuse this code in my own project?
>
Sharp eyes!
There is some redundancy at the moment because I am part way through a
separation of the code that deals strictly with Java reflection
(PropertyUtils) and the code that will deal with conversion to and from
request parameters (BeanUtils). At the moment, PropertyUtils is looking
pretty solid -- BeanUtils will shortly be shedding the methods that are
duplications, which should be completed in time for the 1.0 release I'd like
to have by ApacheCon Europe (Oct 23-25).
If there are particular methods in BeanUtils you are planning to use let me
know and I can warn of potential problems.
>
> --
> Curt Hagenlocher
> curth@motek.com
Craig
--
====================
See you at ApacheCon Europe <http://www.apachecon.com>!
Session VS01 (23-Oct 13h00-17h00): Sun Technical Briefing
Session T06 (24-Oct 14h00-15h00): Migrating Apache JServ
Applications to Tomcat