cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Steven Dolg <steven.d...@indoqa.com>
Subject Re: [VOTE] Fix for COCOON3-79
Date Mon, 31 Oct 2011 14:57:59 GMT
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.

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)

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


Mime
View raw message