Struts 2
  1. Struts 2
  2. WW-2896

Various generics and StringBuilder fixes

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.1.2
    • Fix Version/s: Future
    • Component/s: Core Actions
    • Labels:
      None
    • Flags:
      Patch
    1. variousgenericsbuilder.patch
      22 kB
      Mathias Bogaert
    2. variousgenericsbuilder.patch
      36 kB
      Mathias Bogaert

      Activity

      Hide
      Mathias Bogaert added a comment -

      More fixes.

      Show
      Mathias Bogaert added a comment - More fixes.
      Hide
      Wes Wannemacher added a comment -

      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.

      1.
      First off, in the first patch, posted 11/27, this change to Component.java-

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

      2.
      In TagModel.java, 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

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

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

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

      6.
      In ContainUtil.java, 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.

      Show
      Wes Wannemacher added a comment - 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. 1. First off, in the first patch, posted 11/27, this change to Component.java- 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. 2. In TagModel.java, 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 3. 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. 4. 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) 5. In MessageStoreInterceptor, it seems like there are a lot of whitespace changes, which I like to avoid committing (since it generates email spam) 6. In ContainUtil.java, 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.
      Hide
      Dave Newton added a comment -

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

      Oh, were the StringBuilder patches only partially applied? I fixed PortletActionRedirectResult yesterday and discovered IDEA is removing blank chars at the end of lines so the commit generated a bunch of noise. I know it's noise, but... meh.

      I'm not a big fan of 78-chars-per-line these days, either not practical in real Java code. We can move this to dev list to continue.

      Show
      Dave Newton added a comment - 4. 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) Oh, were the StringBuilder patches only partially applied? I fixed PortletActionRedirectResult yesterday and discovered IDEA is removing blank chars at the end of lines so the commit generated a bunch of noise. I know it's noise, but... meh. I'm not a big fan of 78-chars-per-line these days, either not practical in real Java code. We can move this to dev list to continue.
      Hide
      musachy added a comment -

      moving to 2.1.4. The s/StringBuffer/StringBuilder changes are fine, but using generics all over the place is something we can do on 2.1.4

      Show
      musachy added a comment - moving to 2.1.4. The s/StringBuffer/StringBuilder changes are fine, but using generics all over the place is something we can do on 2.1.4
      Hide
      musachy added a comment -

      moving to future, this is a long task and it is not something we need to do right away, volunteers welcome

      Show
      musachy added a comment - moving to future, this is a long task and it is not something we need to do right away, volunteers welcome

        People

        • Assignee:
          Unassigned
          Reporter:
          Mathias Bogaert
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:

            Development