cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Francesco Chicchiriccò <ilgro...@apache.org>
Subject Re: [VOTE] Fix for COCOON3-79
Date Mon, 31 Oct 2011 17:09:17 GMT
On 31/10/2011 16:12, Francesco Chicchiriccò wrote:
> On 31/10/2011 15:57, Steven Dolg wrote:
>> [...]
>>
>> However, I really dislike some of the coding style in this patch.
>> Specifically declaring local variables without assignment and/or 
>> declaring them outside the one block they are used in (in most cases 
>> this is the same thing since people finally stopped declaring all 
>> variables at the beginning of a method)
>>
>> e.g.
>> Object resolvedValue;
>> for (Entry<String, String> entry : this.getParameters().entrySet()) {
>>     resolvedValue = invocation.resolveParameter(entry.getValue());
>>     parameters.put(entry.getKey(), resolvedValue);
>> }
>>
>> It does not result in any improved runtime performance, adds another 
>> line of code and makes the variable visible after its intended lifetime.
>> TBH, I cannot think of a single advantage of this coding style.
>
> You are right: I will review the patch code in order to remove this 
> kind of horrible mistakes.
>
>> Furthermore this patch seems to introduce quite some additional 
>> duplication and complexity.
>> (I'm not trying to imply that the code was flawless before, just that 
>> duplicating 8 lines of code a couple of times is worse than 
>> duplicating 2 lines of code a couple of times and that having 3 "ifs" 
>> in a method is worse than having 2)
>
> Could you please be more specific here? Which code lines are you 
> referring to?

Ok, I guess you are referring to GenerateNode's, TransformNode's and 
ReadNode's invoke(): once again, you are right but I would rephrase your 
objection "Furthermore this patch seems to introduce quite some 
additional duplication and complexity." as "Furthermore this patch seems 
not to fix quite some additional duplication and complexity".

Actually, I was there only changing calls from String to Object and I 
did not realize how much duplication was in there; anyway, I've just 
uploaded an updated patch to fix this as well. Thanks for reporting.

Regards.

>> I still don't like variable names like "sbResult", but we've been 
>> there. No need to revisit.
>>
>>
>> While I think the improvements to functionality are well worth it and 
>> I wouldn't mind terribly seeing this applied,
>> I don't think this patch actually improves the codebase of Cocoon.
>
> Of course, I don't agree about this: only to take a very simple 
> reference, without the patch StringTemplate's $if$ won't barely work. 
> This means codebase improvement, IMHO.
-- 
Francesco Chicchiriccò

Apache Cocoon Committer and PMC Member
http://people.apache.org/~ilgrosso/


Mime
View raw message