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 [EMAIL PROTECTED] 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: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
