Take the 2-minute tour ×
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:

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.
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 '13 at 2:40

1 Answer 1

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.