Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.6
    • Component/s: Engine
    • Labels:
      None

      Description

      velocityCount = Used in the #foreach() directive, defines the string to be used as the context key for the loop count. A template would access the loop count as $velocityCount.

      Knowing current version of the loop counter is very useful, but something is missing. Very often, when you generate some content base on a model(JavaScript code based on some UI model) you have the need to know if this is the last time when the #foreach block will be executed or not.

      Example

      You have a collection of objects Property(bean with key, value fields) and you want to generate a JavaScript object with them. The result should be like this :

      {id : 1 , name = "John", ...}

      you can generate this with

      {
      #foreach($property in $properties)
      $property.key : "$property.value" #if($velocityCount != $properties.size()) , #end
      #end
      }

      You can store $properties.size() of course outside the loop.

      The template will be less verbose if I will have something like that. #if($velocityHasNext) , #end.

      Instead of saving only the counter, a new variable called velocityHasNext whould be populated with the result of iterator.hasNext(). This is a minor modification of current #foreach directive

      while (!maxNbrLoopsExceeded && i.hasNext())
      {
      // TODO: JDK 1.4+ -> valueOf()
      context.localPut(counterName , new Integer(counter));
      context.localPut(hasNextName , i.hasNext()); <--- here is the change
      Object value = i.next();
      context.localPut(elementKey, value);

      /*

      • If the value is null, use the special null holder context
        */
        if( value == null )
        Unknown macro: { if( nullHolderContext == null ) { // lazy instantiation nullHolderContext = new NullHolderContext(elementKey, context); } node.jjtGetChild(3).render(nullHolderContext, writer); }

        else

        { node.jjtGetChild(3).render(context, writer); }

        counter++;

      // Determine whether we're allowed to continue looping.
      // ASSUMPTION: counterInitialValue is not negative!
      maxNbrLoopsExceeded = (counter - counterInitialValue) >= maxNbrLoops;
      }

      also init should contain

      public void init(RuntimeServices rs, InternalContextAdapter context, Node node)
      throws TemplateInitException

      { super.init(rs, context, node); counterName = rsvc.getString(RuntimeConstants.COUNTER_NAME); hasNextName = rsvc.getString(RuntimeConstants.HAS_NEXT_NAME); counterInitialValue = rsvc.getInt(RuntimeConstants.COUNTER_INITIAL_VALUE); .... }

      This should help creating clear templates(and avoid some mistakes, sometime - like using the wrong collection to test ).

      Thanks.

        Issue Links

          Activity

          Adrian Tarau created issue -
          Hide
          Adrian Tarau added a comment -

          .. current value of the loop counter ...

          Show
          Adrian Tarau added a comment - .. current value of the loop counter ...
          Hide
          Nathan Bubna added a comment -

          I also wanted something like this, but rather than build it into the core, i made this:

          http://velocity.apache.org/tools/devel/javadoc/org/apache/velocity/tools/generic/LoopTool.html

          This does all this:

          #foreach( $item in $loop.watch($items) )
          #if( $loop.isFirst() ) first! #end
          count (1-based): $loop.count
          index (0-based): $loop.index
          #if( $loop.isLast() ) last! #end
          #end

          and more. Since it is generic, you need only context.put("loop", new LoopTool()) to use it.

          Show
          Nathan Bubna added a comment - I also wanted something like this, but rather than build it into the core, i made this: http://velocity.apache.org/tools/devel/javadoc/org/apache/velocity/tools/generic/LoopTool.html This does all this: #foreach( $item in $loop.watch($items) ) #if( $loop.isFirst() ) first! #end count (1-based): $loop.count index (0-based): $loop.index #if( $loop.isLast() ) last! #end #end and more. Since it is generic, you need only context.put("loop", new LoopTool()) to use it.
          Hide
          Adrian Tarau added a comment -

          Looks fine, but I think something like this should be into the core, part of #foreach implementation. The modifications are minor and I think this template

          { #foreach($property in $properties) $property.key : "$property.value" #if($velocityHasNext) , #end #end }

          looks better than

          { #foreach($property in $loop.watch($properties)) $property.key : "$property.value" #if($loop.isLast()) , #end #end }

          I agree, something like that could offer also isFirst, but exposing the iterator hasNext is more that you need in general. Also having a tool on top of the original collection brings : a new object is created, additional calls are performed. Nothing major, but inside the directive you already have access of it.hasNext() and there is only one additional line : context.localPut(hasNextName , i.hasNext());

          This is also similar with the new way to iterate collections in Java 5

          for (Item item : items) {
          System.out.print(item.toString());
          if (...)

          { <-- how can I check if it is the last time or not 'cause I want to print a new line only if is not the last element? I don't have access to the iterator(they always use an iterator to implement this behind the scene). System.out.print("\n"); }


          }

          Show
          Adrian Tarau added a comment - Looks fine, but I think something like this should be into the core, part of #foreach implementation. The modifications are minor and I think this template { #foreach($property in $properties) $property.key : "$property.value" #if($velocityHasNext) , #end #end } looks better than { #foreach($property in $loop.watch($properties)) $property.key : "$property.value" #if($loop.isLast()) , #end #end } I agree, something like that could offer also isFirst, but exposing the iterator hasNext is more that you need in general. Also having a tool on top of the original collection brings : a new object is created, additional calls are performed. Nothing major, but inside the directive you already have access of it.hasNext() and there is only one additional line : context.localPut(hasNextName , i.hasNext()); This is also similar with the new way to iterate collections in Java 5 for (Item item : items) { System.out.print(item.toString()); if (...) { <-- how can I check if it is the last time or not 'cause I want to print a new line only if is not the last element? I don't have access to the iterator(they always use an iterator to implement this behind the scene). System.out.print("\n"); } }
          Hide
          Adrian Tarau added a comment -

          context.localPut(hasNextName , i.hasNext()) creates anyway a new Boolean object

          Anyway, template syntax looks better with $velocityHasNext instead of $tool.isLast() + $tool.wrap($collection).

          Show
          Adrian Tarau added a comment - context.localPut(hasNextName , i.hasNext()) creates anyway a new Boolean object Anyway, template syntax looks better with $velocityHasNext instead of $tool.isLast() + $tool.wrap($collection).
          Hide
          Nathan Bubna added a comment -

          I'm not at all opposed to $velocityHasNext. I was just offering my preferred solution. For me, the object creation and extra calls are negligible and which "looks better" is entirely subjective. You and i, at least, seem to have different opinions. I also generally use the shorthand $loop.last and like that i can have $loop.first, $loop.last, $loop.index, $loop.count, and other interesting features available when i want them, with no cost if i don't use them. And since Tools 2 lazy loads tools, $loop itself also costs nothing until i use it.

          But really, $velocityHasNext sounds fine and cheap to me. Care to turn your code above into a patch for easy application?

          Show
          Nathan Bubna added a comment - I'm not at all opposed to $velocityHasNext. I was just offering my preferred solution. For me, the object creation and extra calls are negligible and which "looks better" is entirely subjective. You and i, at least, seem to have different opinions. I also generally use the shorthand $loop.last and like that i can have $loop.first, $loop.last, $loop.index, $loop.count, and other interesting features available when i want them, with no cost if i don't use them. And since Tools 2 lazy loads tools, $loop itself also costs nothing until i use it. But really, $velocityHasNext sounds fine and cheap to me. Care to turn your code above into a patch for easy application?
          Hide
          Adrian Tarau added a comment -

          I agree, "looks better" is subjective, but I'm using Velocity so we have something in common

          I have nothing against using a tool, you can always activate your tool to gain any (extra) functionality above core functionality. For me, in this case, it seems like a good candidate for a core functionality.

          Don't mind if I do I will post a patch against HEAD/trunk. I presume nothing changed here lately, so this patch could be applied on 1.5 and above without any problems.

          Thanks Nathan.

          Show
          Adrian Tarau added a comment - I agree, "looks better" is subjective, but I'm using Velocity so we have something in common I have nothing against using a tool, you can always activate your tool to gain any (extra) functionality above core functionality. For me, in this case, it seems like a good candidate for a core functionality. Don't mind if I do I will post a patch against HEAD/trunk. I presume nothing changed here lately, so this patch could be applied on 1.5 and above without any problems. Thanks Nathan.
          Hide
          Adrian Tarau added a comment -

          By the way, any suggestion for a default name?

          $velocity + Count and $velocity+ HasNext - I thought to keep the current notation, with velocity as a prefix.

          Show
          Adrian Tarau added a comment - By the way, any suggestion for a default name? $velocity + Count and $velocity+ HasNext - I thought to keep the current notation, with velocity as a prefix.
          Hide
          Nathan Bubna added a comment -

          sounds fine to me.

          Show
          Nathan Bubna added a comment - sounds fine to me.
          Hide
          Adrian Tarau added a comment -

          Works like a charm

          Show
          Adrian Tarau added a comment - Works like a charm
          Adrian Tarau made changes -
          Field Original Value New Value
          Attachment VELOCITY-600.patch [ 12383725 ]
          Hide
          Ilkka Priha added a comment -

          One of the good design principles in Velocity core has been to not to reserve hard-coded keywords from the context. To follow this principle, $velocityHasNext should be configurable in the same way as $velocityCount is.
          – Ilkka

          Show
          Ilkka Priha added a comment - One of the good design principles in Velocity core has been to not to reserve hard-coded keywords from the context. To follow this principle, $velocityHasNext should be configurable in the same way as $velocityCount is. – Ilkka
          Hide
          Adrian Tarau added a comment -

          It is, this patch follows the same (initialization/usage) rules as $velocityCount

          Show
          Adrian Tarau added a comment - It is, this patch follows the same (initialization/usage) rules as $velocityCount
          Hide
          Christopher Schultz added a comment -

          While we're on the topic, I've never known how to access the count for an outer loop. Something like:

          #foreach($foo in $foos)
          #foreach($bar in $bars)
          Looking at \$bars[$velocityCount] in \$foos[$outerVelocityCount]
          #end
          #end

          AFAIK, there's no way to get the "outer" count inside the loop unless you explicitly set a variable. Would this be useful at all?

          I'm inclined to agree with Nathan that a tool is the answer because this capability is not entirely necessary and adds overhead (albeit negligible). Tools are configurable at runtime and therefore are only available if your application actually needs them. Just my opinion FWIW.

          Show
          Christopher Schultz added a comment - While we're on the topic, I've never known how to access the count for an outer loop. Something like: #foreach($foo in $foos) #foreach($bar in $bars) Looking at \$bars [$velocityCount] in \$foos [$outerVelocityCount] #end #end AFAIK, there's no way to get the "outer" count inside the loop unless you explicitly set a variable. Would this be useful at all? I'm inclined to agree with Nathan that a tool is the answer because this capability is not entirely necessary and adds overhead (albeit negligible). Tools are configurable at runtime and therefore are only available if your application actually needs them. Just my opinion FWIW.
          Hide
          Adrian Tarau added a comment -

          I don't think there is any solution other than an additional variable.

          Based on my experience with Velocity(generating code/script - Java, JavaScript, CSS) it is common to iterate and do something different at the end(or skip doing what you did for N - 1 elements).

          I barely used the counter, except if you want to count and print an index for what you generate or access another List(which I try to avoid). Using the counter as an index to access a collection is wrong(with some exceptions) cause you can access a LinkedList with an index which hits performance.

          As you already said the overhead is negligible(just an additional line of code - except the init section) and it expose the iterator, which is a standard way to work with a collection. You can always activate any tool you want to add extra functionality to #foreach but this feature doesn't hurt at all in my opinion.

          Just my 2 cents

          Show
          Adrian Tarau added a comment - I don't think there is any solution other than an additional variable. Based on my experience with Velocity(generating code/script - Java, JavaScript, CSS) it is common to iterate and do something different at the end(or skip doing what you did for N - 1 elements). I barely used the counter, except if you want to count and print an index for what you generate or access another List(which I try to avoid). Using the counter as an index to access a collection is wrong(with some exceptions) cause you can access a LinkedList with an index which hits performance. As you already said the overhead is negligible(just an additional line of code - except the init section) and it expose the iterator, which is a standard way to work with a collection. You can always activate any tool you want to add extra functionality to #foreach but this feature doesn't hurt at all in my opinion. Just my 2 cents
          Hide
          Nathan Bubna added a comment -

          The LoopTool makes it pretty easy to get first/last/count/index information for outer loops:

          #foreach( $foo in $loop.watch($foos, 'foo') )
          #foreach( $bar in $loop.watch($bars) )
          Looking at \$bars[$loop.count] in \$foos[$loop.getCount('foo')]
          #end
          #end

          This approach works for any depth of nesting, and you don't actually even need the LoopTool to watch the innermost #foreach for you to have this work.

          And i agree with Adrian that using the counter to access other collections is not the best. This is another reason for the LoopTool, as it makes it easy to sync iteration over parallel collections.

          #foreach( $foo in $loop.sync($foos, $bars) )
          $foo is in sync with $loop.synced #default name for sync'ed iterator is "synched"
          #end

          Hope ya'll don't mind my hijacking this conversation for some LoopTool education, but really these are the sort of problems it is aimed at.

          Show
          Nathan Bubna added a comment - The LoopTool makes it pretty easy to get first/last/count/index information for outer loops: #foreach( $foo in $loop.watch($foos, 'foo') ) #foreach( $bar in $loop.watch($bars) ) Looking at \$bars [$loop.count] in \$foos [$loop.getCount('foo')] #end #end This approach works for any depth of nesting, and you don't actually even need the LoopTool to watch the innermost #foreach for you to have this work. And i agree with Adrian that using the counter to access other collections is not the best. This is another reason for the LoopTool, as it makes it easy to sync iteration over parallel collections. #foreach( $foo in $loop.sync($foos, $bars) ) $foo is in sync with $loop.synced #default name for sync'ed iterator is "synched" #end Hope ya'll don't mind my hijacking this conversation for some LoopTool education, but really these are the sort of problems it is aimed at.
          Hide
          Nathan Bubna added a comment -

          Thanks, Adrian. The patch works great, only watch out for Java 5-isms next time, like the autoboxing of that boolean.

          Show
          Nathan Bubna added a comment - Thanks, Adrian. The patch works great, only watch out for Java 5-isms next time, like the autoboxing of that boolean.
          Nathan Bubna made changes -
          Fix Version/s 1.5.1 [ 12312375 ]
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Fix Version/s 1.6 [ 12310290 ]
          Hide
          Adrian Tarau added a comment -

          Thanks Nathan.

          My bad, I've used only Java 1.5 in the last year and I got used with generics, auto boxing, etc.

          Any chance to apply this patch on 1.5.X? It doesn't touch existing code, just add something. I presume 1.5.1(I see 4-5 issues there) will be release before 1.6, right?

          Show
          Adrian Tarau added a comment - Thanks Nathan. My bad, I've used only Java 1.5 in the last year and I got used with generics, auto boxing, etc. Any chance to apply this patch on 1.5.X? It doesn't touch existing code, just add something. I presume 1.5.1(I see 4-5 issues there) will be release before 1.6, right?
          Hide
          Nathan Bubna added a comment -

          If someone decides to roll a 1.5.1 release, then i'd be fine with throwing this patch in too. Feel free to re-unresolve this with 1.5.1 as the fix version, so that it isn't forgotten.

          However, a 1.6 release is just as likely (if not more likely) to be the next thing out the door. Both 1.5.1 and 1.6 are hung up on the same bug (VELOCITY-537). Once that is fixed i will put my energy toward a 1.6 release, as that has features i'm eager for (vararg support!). Anyone who wants to push a 1.5.1 release is welcome to do so, but i'm not sure if anyone will with 1.6 so imminent. And in either case, i think 537 needs fixing first.

          Show
          Nathan Bubna added a comment - If someone decides to roll a 1.5.1 release, then i'd be fine with throwing this patch in too. Feel free to re-unresolve this with 1.5.1 as the fix version, so that it isn't forgotten. However, a 1.6 release is just as likely (if not more likely) to be the next thing out the door. Both 1.5.1 and 1.6 are hung up on the same bug ( VELOCITY-537 ). Once that is fixed i will put my energy toward a 1.6 release, as that has features i'm eager for (vararg support!). Anyone who wants to push a 1.5.1 release is welcome to do so, but i'm not sure if anyone will with 1.6 so imminent. And in either case, i think 537 needs fixing first.
          Hide
          Adrian Tarau added a comment -

          Not sure now to "re-unresolve"

          Show
          Adrian Tarau added a comment - Not sure now to "re-unresolve"
          Hide
          Nathan Bubna added a comment -

          Re-opening so this can also be added to 1.5.1, should such a release ever happen.

          Show
          Nathan Bubna added a comment - Re-opening so this can also be added to 1.5.1, should such a release ever happen.
          Nathan Bubna made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Nathan Bubna made changes -
          Fix Version/s 1.5.1 [ 12312375 ]
          Hide
          Adrian Tarau added a comment -

          Got it

          Show
          Adrian Tarau added a comment - Got it
          Hide
          Christopher Schultz added a comment -

          Might I suggest that in RuntimeConstants the constant HAS_NEXT_NAME be called instead COUNTER_HAS_NEXT_NAME to conform to the other #foreach-related property keys?

          Also, directive.foreach.iterator.name seems a bit confusing. It's not the name of the iterator... it's the name of the boolean that will be set to 'hasNext' during the loop. Why not use directive.foreach.hasNext.name to be consistent?

          Show
          Christopher Schultz added a comment - Might I suggest that in RuntimeConstants the constant HAS_NEXT_NAME be called instead COUNTER_HAS_NEXT_NAME to conform to the other #foreach-related property keys? Also, directive.foreach.iterator.name seems a bit confusing. It's not the name of the iterator... it's the name of the boolean that will be set to 'hasNext' during the loop. Why not use directive.foreach.hasNext.name to be consistent?
          Hide
          Adrian Tarau added a comment -

          I agree with directive.foreach.hasNext.name or directive.foreach.hasnext.name (I think Java naming for configuration keys looks a little bit ... not right) instead of directive.foreach.iterator.name but for HAS_NEXT_NAME it should really be FOREACH_HAS_NEXT_NAME instead of COUNTER_HAS_NEXT_NAME because it doesn't have anything with the #foreach counter (and maybe rename also COUNTER_NAME to FOREACH_COUNTER_NAME in 1.6)

          Show
          Adrian Tarau added a comment - I agree with directive.foreach.hasNext.name or directive.foreach.hasnext.name (I think Java naming for configuration keys looks a little bit ... not right) instead of directive.foreach.iterator.name but for HAS_NEXT_NAME it should really be FOREACH_HAS_NEXT_NAME instead of COUNTER_HAS_NEXT_NAME because it doesn't have anything with the #foreach counter (and maybe rename also COUNTER_NAME to FOREACH_COUNTER_NAME in 1.6)
          Hide
          Will Glass-Husain added a comment -

          Cool patch. Note: we need to update the user guide docs (in velocity/docs project) before closing this.

          Show
          Will Glass-Husain added a comment - Cool patch. Note: we need to update the user guide docs (in velocity/docs project) before closing this.
          Hide
          Nathan Bubna added a comment -

          Ok, it's in the documentation now.

          Show
          Nathan Bubna added a comment - Ok, it's in the documentation now.
          Nathan Bubna made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 1.5.1 [ 12312375 ]
          Resolution Fixed [ 1 ]
          Jarkko Viinamäki made changes -
          Link This issue relates to VELOCITY-658 [ VELOCITY-658 ]
          Mark Thomas made changes -
          Workflow jira [ 12432870 ] Default workflow, editable Closed status [ 12551967 ]
          Mark Thomas made changes -
          Workflow Default workflow, editable Closed status [ 12551967 ] jira [ 12552345 ]

            People

            • Assignee:
              Unassigned
              Reporter:
              Adrian Tarau
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development