Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I often need to loop through a list of items and need both the index position, and the item itself. Typically:

set names {John Paul George Ringo}
set i 0
foreach name $names {
    puts "$i - $name"
    incr i
} 

Output:

0 - John
1 - Paul
2 - George
3 - Ringo

Since I frequently do this, I decided to implement my own loop and call it for_item_index for lack of creativity. Here is my loop and a short code segment to test it:

proc for_index_item {index item iterable body } {
    uplevel 1 set $index 0
    foreach x $iterable {
        uplevel 1 set $item $x
        uplevel 1 $body
        uplevel 1 incr $index
    }
}

# Test it
set names {John Paul George Ringo}
for_index_item i name $names {
    puts "$i - $name"
}

I have tested it with break, and continue and found my new loop performs as expected. My concern is the excessive use of uplevel command in the code. I am seeking reviewers to give me tips for improving it.

Here are my own review of my code:

  • Excessive use of uplevel
  • The index always starts at zero. There are times when I want it to start at 1 or some other values. To add that feature, I will probably introduce another parameter, startValue
  • Likewise, the index always get incremented by 1. The user might want to increment it by a different values such as 2, or -1 to count backward. Again, introducing another parameter, step might help, but at this point, the loop is getting complicated.

Update

I looked over Glenn Jackman's suggestion and like it. However, due to my failure to describe the problem, his solution is not quite working the way I like. Basically, I like to iterate over all items and not skipping any of them. Secondly, what I called index should have been sequenced number, which has nothing to do with the index within the list. For example, may be we want the output to be:

10 - John
20 - Paul
30 - George
40 - Ringo

In which case, we still traverse all items in the list, but the sequence is different from the default 0..3. I also like your solution for making it simple to parse the parameters and the flags are Tcl-like. Before I was reading Glenn's solution, I already came up with my own enhancements, which includes the start- and increment values:

proc for_index_item {indexexpr itemvar iterable body} {
    # Break down indexexpr
    set step 1
    set start 0
    if {[llength $indexexpr] == 3} {
        set step [lindex $indexexpr 2]
    }
    if {[llength $indexexpr] >= 2} {
        set start [lindex $indexexpr 1]
    }
    upvar 1 [lindex $indexexpr 0] index
    set index $start

    upvar 1 $itemvar item

    # Actual loop
    foreach item $iterable {
        uplevel 1 $body
        incr index $step
    }
}

#
# Test
#

puts "\nDefault, start with 0"
set names {John Paul George Ringo}
for_index_item i name $names {
    puts "  $i - $name"
}

puts "\nStarts from 1"
set names {John Paul George Ringo}
for_index_item {i 1} name $names {
    puts "  $i - $name"
}

puts "\nWith start value and increment"
set names {John Paul George Ringo}
afor_index_item {i 110 10} name $names {
    puts "  $i - $name"
}

In my scenario, an index (I will need to rename it to remove confusion) can be one of these forms:

  • A single var, e.g. i
  • A list of var name and start value, e.g. {i 1}
  • A list of var name, start value and increment, e.g. {i 4 -1}

As Glenn predicted, a great deal of effort goes into parsing the parameters. At this point, I am happy with the current version and start using it in my code.

Update 2

I applied Glenn's suggestion for parsing the index expression, rename index to sequence. The code is looking much cleaner now. The only caveat is in some systems at work, we are still running Tcl 8.4, which is why I fallback on Tclx:

if {[catch package require Tcl 8.5]} {
    package require Tclx
}

proc for_sequence_item {sequenceExpression itemVar iterable body} {
    # Break down sequenceExpression
    lassign $sequenceExpression sequenceVar start step
    if {$start == ""} {set start 0}
    if {$step == ""} {set step 1}
    upvar 1 $sequenceVar sequence
    upvar 1 $itemVar item
    set sequence $start

    # Actual loop
    foreach item $iterable {
        uplevel 1 $body
        incr sequence $step
    }
}
share|improve this question
Here's a bit more concise way to "Break down indexexpr": lassign $indexexpr idxvar start step; if {$start == ""} {set start 0}; if {$step == ""} {set step 1} (shame about comment code formatting) – glenn jackman Jun 21 at 2:40

1 Answer

I would base something like this on for rather than foreach

proc foreach_with_index {idxvar elemvar list body args} {
    array set opts {-start 0 -inc 1}
    if {[llength $args] > 0 && [llength $args]%2 == 0} {array set opts $args}
    upvar 1 $idxvar idx
    upvar 1 $elemvar elem
    for {set idx $opts(-start)} {$idx < [llength $list]} {incr idx $opts(-inc)} {
        set elem [lindex $list $idx]
        uplevel 1 $body
    }
}

So you get

% foreach_with_index i e {a b c d e} {puts "$i - $e"}
0 - a
1 - b
2 - c
3 - d
4 - e
% foreach_with_index i e {a b c d e} {puts "$i - $e"} -start 1 -inc 2
1 - b
3 - d

Some thoughts:

  • don't forget about upvar to link a variable up the call stack.
  • I chose to use Tk style -options for some reason. It looks a bit awkward, but it's not exactly pretty to have to jam arguments at the end. You might choose to arguments with default values for start and inc. Or you might decide to put optional arguments in the middle of the list somewhere (like the way puts has an optional filehandle as the 1st arg), but that's more work to parse the arguments.
  • in hindsight, using for vs foreach is a pretty arbitrary decision. Here, with for, you need to extract the element from the array. With foreach you need to increment the index.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.