Re: Not changing things that work (was Re: svn commit: r165502 - /cocoon/trunk/src/java/org/apache/cocoon/transformation/TraxTransforme r.java)

[Antonio Gallardo <ag...@agssa.net>]

On Mar, 3 de Mayo de 2005, 11:45, Sylvain Wallez dijo:
> Vadim Gritsenko wrote:
>
>> antonio@apache.org wrote:
>>
>>>
>>> Use BooleanUtils, use regexp for checking valid XSLT parameter names,
>>> fix docs.
>>>
>>> Modified:
>>> cocoon/trunk/src/java/org/apache/cocoon/transformation/TraxTransformer.java
>>>
>>> @@ -196,7 +199,10 @@
>>>
>>>      /** Exception that might occur during setConsumer */
>>>      private SAXException exceptionDuringSetConsumer;
>>> -    +
>>> +    /** Check if an expression is a valid XSLT Parameter Name **/
>>> +    private static final RE reValidXSLTParameterName = new
>>> RE("^[\\w][\\w\\d\\.-]*");
>>> +
>>>      /**
>>>       * Configure this transformer.
>>>       */
>>
>>
>> From RE Javadoc:
>>
>>  *    \w        Matches a "word" character (alphanumeric plus "_")
>>
>> \w includes numbers, so this RE is incorrect.
>>
>>
>>> @@ -483,26 +490,7 @@
>>>       * Test if the name is a valid parameter name for XSLT
>>>       */
>>>      static boolean isValidXSLTParameterName(String name) {
>>> -        if (name.length() == 0) {
>>> -            return false;
>>> -        }
>>> -
>>> -        char c = name.charAt(0);
>>> -        if (!(Character.isLetter(c) || c == '_')) {
>>> -            return false;
>>> -        }
>>> -
>>> -        for (int i = name.length()-1; i > 1; i--) {
>>> -            c = name.charAt(i);
>>> -            if (!(Character.isLetterOrDigit(c) ||
>>> -                c == '-' ||
>>> -                c == '_' ||
>>> -                c == '.')) {
>>> -                return false;
>>> -            }
>>> -        }
>>> -
>>> -        return true;
>>> +        return reValidXSLTParameterName.match(name);
>>>      }
>>>
>>>      /**
>>
>>
>> Given that org.apache.regexp.RE uses same test for \w:
>>
>>   ...
>>   (Character.isLetterOrDigit(c) || c == '_')
>>   ...
>>
>> And given that RE has more overhead, I wonder how many times slower
>> this new test is. What is improved, then? I don't think this snippet
>> had any bugs in it, and it was faster, so why not leave it as it is?
>
>
> Yes, why?

I just wanted to make the code more readable. I am reverting this now. Sorry.

Best Regards,

Antonio Gallardo

>
> Antonio, we know how much you love commons-lang, but really, why
> spending your and other's energy to add this additional dependency to
> existing code that has been running smoothly for years?
>
> If it ain't broken, don't fix it!
>
> Sylvain
>
> --
> Sylvain Wallez                        Anyware Technologies
> http://apache.org/~sylvain            http://anyware-tech.com
> Apache Software Foundation Member     Research & Technology Director
>