Mailing-List: contact dev-help@cocoon.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cocoon.apache.org
Received-SPF: neutral (hermes.apache.org: local policy)
Message-ID: <4277AAC7.5080202@apache.org>
Date: Tue, 03 May 2005 18:45:59 +0200
From: Sylvain Wallez <sylvain@apache.org>
Organization: Anyware Technologies
User-Agent: Mozilla Thunderbird 1.0 (Macintosh/20041206)
MIME-Version: 1.0
To: dev@cocoon.apache.org
Subject: Not changing things that work (was Re: svn commit: r165502 -
 /cocoon/trunk/src/java/org/apache/cocoon/transformation/TraxTransformer.java)
References: <20050501133114.10029.qmail@minotaur.apache.org>
 <42776B00.5060404@reverycodes.com>
In-Reply-To: <42776B00.5060404@reverycodes.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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