You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cocoon.apache.org by Sylvain Wallez <sy...@apache.org> on 2005/05/03 18:45:59 UTC
Not changing things that work (was Re: svn commit: r165502 - /cocoon/trunk/src/java/org/apache/cocoon/transformation/TraxTransformer.java)
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
Re: Not changing things that work (was Re: svn commit: r165502 -
/cocoon/trunk/src/java/org/apache/cocoon/transformation/TraxTransforme
r.java)
Posted by 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
>