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/
