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 15:12:05 GMT
On 31/10/2011 15:57, Steven Dolg wrote:
> Am 31.10.2011 14:19, schrieb Francesco Chicchiriccò:
>> Hi all,
>> I've developed a fix for COCOON3-79, attached [1] as a patch.
>> Some background information is available at [2].
>>
>> Since the aforementioned patch will involve changing some interface, 
>> I'm calling the vote on Code Modification [3]:
>> +1 = Yes, apply patch attached to [1]
>> -1 = No, don't apply patch attached to [1] (with justification for 
>> the veto)
>
> I'm generally in favor of increasing potential and power of Cocoon 3.
> Adding more flexibility is great. The patch appears to be of 
> reasonable size for the changes it introduces.
>
>
> 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?

> 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.

Regards.

> So my vote is really a 0.
>
>
>>
>> Please cast your votes before Fri 4 Nov 06:00 UTC.
>>
>> Here is mine:
>> +1
>>
>> Regards.
>>
>> [1] https://issues.apache.org/jira/browse/COCOON3-79
>> [2] http://www.mail-archive.com/dev@cocoon.apache.org/msg59948.html
>> [3] http://www.apache.org/foundation/voting.html
>>
>


-- 
Francesco Chicchiriccò

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


Mime
View raw message