cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sylvain Wallez <sylv...@apache.org>
Subject Not changing things that work (was Re: svn commit: r165502 - /cocoon/trunk/src/java/org/apache/cocoon/transformation/TraxTransformer.java)
Date Tue, 03 May 2005 16:45:59 GMT
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?

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


Mime
View raw message