Hi everyone, I am seeking community input on a proposed code modification in https://github.com/apache/commons-beanutils/pull/364. The PR simplifies two classes, removing about 33 lines of code.
Gary Gregory reviewed the proposed code modification: > -1 This is terrible IMO for two reasons: 1) the one-step switch logic > has devolved into a cascading if-else and 2) equals() is replaced by > the ignore case version. I appreciate the feedback and responded in the PR with some advantages of the approach, along with acknowledgments of potential disadvantages and why I believe the code modification is a net benefit: > Hi @garydgregory, thanks for your feedback. While your comment > accurately describes the changes in this PR, it doesn't explain why > they're problematic. To clarify, here are some advantages of the > proposed approach: > > - Reduces code volume compared to the original, enhancing readability > and maintainability. > - Avoids extra variables and method calls for string normalization, > eliminating minor indirection that can hinder readability. > - Eliminates upfront string creation: equalsIgnoreCase() compares > characters on the fly without allocating a new string, unlike > toLowerCase() (which can incur memory and copy overhead). > - Steers clear of premature optimization, which is generally > discouraged. > > That said, the original switch approach does indeed benefit from > Java's hash-based lookup and performs normalization only once (both > improving performance). However, this optimization matters mainly for > large inputs or hot code paths, and such optimizations were likely > more critical in 2005 than today. Moreover, multiple > .equalsIgnoreCase() calls still involve repeated comparisons, just > like the multiple .equals() calls in convertToType, but with only > slightly higher per-comparison cost, making the new approach not much > worse than the original. I am bringing this to the list for broader discussion. What are your thoughts on the trade-offs? Does this seem like a worthwhile simplification, or are there concerns I am missing? If we can reach consensus here, I would like to proceed with the change (potentially with adjustments based on feedback). Thanks, Basil --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
