commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject DO NOT REPLY [Bug 18942] - [beanutils] Add "t/f" to BooleanConverter
Date Thu, 03 Mar 2005 12:14:33 GMT
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=18942>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=18942





------- Additional Comments From skitching@apache.org  2005-03-03 13:14 -------
Ok, I've had a closer look at the code.


A number of slightly tricky issues have popped to mind. I'm sure they could all
be resolved, but I am seriously wondering whether it might instead be better to
just improve the documentation on how people can write & register their own
custom converters to override the default behaviours. BeanUtils *does* currently
make it possible for users to do language-specific boolean mappings (by writing
custom Converters), though it certainly isn't as easy or as well documented as
it could be!

Have a look at the issues listed below and let me know what you think.

Please don't think that because the comments are long and complex that I don't
like your patch. I think the issue is reasonable, and worth spending time on : 3
people (Bruno/Mattias/Eric) have requested this feature. But it's just not quite
as simple as it first appears. And of course I'm not a regular BeanUtils
maintainer so I'm trying to be extra careful not to stuff anything up :-)

I'm also wondering whether code that currently uses BeanUtils (eg Struts) has
already dealt with this issue...it might be worth asking on the Struts list
whether they use BeanUtils to process input from non-english languages.

=== How do end users actually use this new functionality?

Were you thinking of something like this?
  BooleanConverter bc = (BooleanConverter) ConvertUtils.lookup(Boolean.class);
  bc.addTrueString("oui");
  bc.addFalseString("non");

or like this?
  BooleanConverter bc = new BooleanConverter();
  bc.addTrueString("oui");
  bc.addFalseString("non");
  ConvertUtils.register(bc, Boolean.class);

If the latter, then it seems *almost* as easy for the user to just write a
custom Converter class of their own that implements their desired mapping than
for BeanUtils to provide a configurable BooleanConverter...

In either case, *just* the conversion of String->Boolean is affected.
String->boolean and String->Boolean[] conversions have not been modified (see below)


=== BooleanArrayConverter

Perhaps BooleanArrayConverter should contain a reference to a BooleanConverter,
with the ConvertUtilsBean.deregister() method ensuring that the
BooleanArrayConverter constructor is passed the just-created BooleanConverter
object? That way, any changes to the BooleanConverter will automatically affect
the BooleanArrayConverter too.

ie in ConvertUtilsBean.deregister(), do something like this:
    register(booleanArray.getClass(),
        new BooleanArrayConverter(bc, booleanArray));
where bc is the same BooleanConverter object that was registered for the
Boolean.class type. That way, changing the true/false values for the
Boolean.class converter will also change the mappings for the BooleanArrayConverter.

Or the documentation for the BooleanConverter can just explain that any calls to
 addTrueString/addFalseString won't affect String->Boolean[] conversions.

Any such change, however, would have to be carefully done to avoid
backwards-compatibility issues. For example, the old constructors must remain in
place.

=== String->boolean

Currently, the ConvertUtilsBean.deregister() method creates separate instances
of BooleanConverter to handle String->Boolean.class (ie String to Boolean) and
String->Boolean.type (ie String to boolean).

Perhaps it would be best to change the ConvertBeanUtils.deregister() method from
       register(Boolean.TYPE, new BooleanConverter(defaultBoolean));
       register(Boolean.class,  new BooleanConverter(defaultBoolean));
   to
       BooleanConverter bc = new BooleanConverter(defaultBoolean);
       register(Boolean.TYPE, bc);
       register(Boolean.class, bc);

=== Thread-safety

I can't see anything in the beanutils javadoc regarding the issue of
threadsafety, but it would appear to me that a Converter is really required to
be thread-safe, as it could be possible for ConvertUtils functionality to be
invoked from multiple threads concurrently, resulting in the same Converter
instance being run concurrently.

I see your patch uses Hashtable rather than HashMap, and Hashtable *is*
threadsafe, though at the price of extra synchronization. The old code was of
course thread-safe because it didn't use any collections to store this data.
[NB: it would be nice to add a comment re *why* you used Hashtable instead of
HashMap'].

Here the approach of having users just create a custom Converter class of their
own with whatever true/false strings they want hard-wired in can make life
easier than the proposed patch. However it *does* fail for apps that want to
detect the language at run-time and load constants from a resource bundle or
similar. So I'm generally in favour of something like your patch that allows a
BooleanConverter to be *configured*. 

I do, however, think it worth considering other options. Objects that are
"mutable" (eg have get/set/add/clear methods) are always potential thread-safety
dangers. Objects whose state is completely defined by their constructor are much
easier to manage. A BooleanConverter class like this might be "safer":
  public class BooleanConverter implements Converter {
    public BooleanConverter(String[] trueStrings, String[] falseStrings, ...) {
    }
    // NO methods to add/remove/modify the true and false strings
  }
In order to change the conversion mappings, users would then use
ConvertUtils.register(bc, Boolean.class) rather than
ConvertUtils.lookup(Boolean.class).addTrueString(...), ie replace the converter
rather than modifying it. I think this would still meet the user requirements
for most people. The major drawback, however, is that it stuffs up my proposal
above for handling BooleanArrayConverter :-(

=== avoid calling addTrueString/addFalseString from getKnownStrings

This is just a minor code issue. The addTrueString/addFalseString methods call
getKnownStrings, so it would seem better in getKnownString to just do:
     knownStrings = new Hashtable();
     knownStrings.put("true", Boolean.TRUE);
     ....

=== Remove existing mappings

I think there definitely needs to be a way to remove the built-in mappings for
BooleanConverter. For example, interpreting "t" as a true value may not be
appropriate in all cases.

-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message