struts-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wes Wannemacher (JIRA)" <>
Subject [jira] Commented: (WW-2896) Various generics and StringBuilder fixes
Date Mon, 01 Dec 2008 03:45:37 GMT


Wes Wannemacher commented on WW-2896:

Mathias, I appreciate your effort, but these patches are rather large... I am going to start
going through them and post a few questions here to illicit feedback from other committers.

First off, in the first patch, posted 11/27, this change to 
-            Map params = getParameters();
             if (value == null) {
-                params.remove(key);
+                getParameters().remove(key);
             } else {
-                params.put(key, value);
+                getParameters().put(key, value);

I'm not a fan of calling a method more than once when a scoped variable will save the method
call. The difference in memory/cpu usage is negligible, but if this section is expanded, having
the variable available seems more appropriate. 

In, the following change - 
-    public Writer getWriter(Writer writer, Map params)
-        throws TemplateModelException, IOException {
+    public Writer getWriter(Writer writer, Map params) throws TemplateModelException, IOException

is not gonna go in because we try to keep lines less than 78 characters[*], there are a few
others in that same file.
*citation needed

In core/src/site/resources/tags/ajax/autocompleter.html, it seems like building core generates
the following change - 

-					<td align="left" valign="top">String</td>
+					<td align="left" valign="top">Integer</td>

I've been seeing that show up today, I haven't committed it, so if anyone knows what's up,
I'd like to know.

The change to PortletActionRedirectResult shows how using generics cleans up code, but the
spacing looks like it got skewed. (Also, it appears that not everyone was keeping with the
78 char width in the trunk copy)

In MessageStoreInterceptor, it seems like there are a lot of whitespace changes, which I like
to avoid committing (since it generates email spam)

In, I am not convinced that adding an 'else' is the right thing to do. Following
the logic of the code, it seems like there is a possibility that an object can be a Map that
does not contain obj2, but obj1 is iterable and does contain obj2. After reading though, it
would seem that the section that checks if obj1 is a Map could be shortened by performing
both tests ( if (obj1 instanceof Map && ((Map) obj1).containsKey(obj2)) return true).

All in all, this was obviously quite a bit of work and I am sure that I am not the only one
that appreciates you doing it. I may bump this to 2.1.4 because I am interested in getting
a 2.1.3 release as quick as possible.

> Various generics and StringBuilder fixes
> ----------------------------------------
>                 Key: WW-2896
>                 URL:
>             Project: Struts 2
>          Issue Type: Improvement
>          Components: Core Actions
>    Affects Versions: 2.1.2
>            Reporter: Mathias Bogaert
>             Fix For: 2.1.3
>         Attachments: variousgenericsbuilder.patch, variousgenericsbuilder.patch

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message