I don't know if this has been brought up, but I just noticed that the each method for Map calls the function given to it with the order of (Value, Key). This is exactly the opposite of how every other language does it:

for key, value in dictionary.items(): ... # Python

dict.each do |key, value| ... end # Ruby

foreach ($dict as $key => $value) { ... } # PHP

They are key-value pairs, not value-key pairs. It is permanently wired into every programmer's brain that the key goes on the right hand side. I think this would be very confusing!

My only guess as to why it is this way is to allow you to loop through either the values or the values and keys with a single method. However, I think it would better either to

1) call f(value) with a single parameter and f(key, value) with two, or 2) split up Map.each into multiple methods, perhaps Map.each (loops through key-value pairs), Map.eachValue (loops through values only), and Map.eachKey (loops through keys only)

How do you guys feel about this?

I've been meaning to ask about that for a while, thanks for saving me the typing.

I don't know the reason behind it but it definitely tripped me as well and I think the order should be | key, value |.

Its "backwards" so that you can leave off the key when you don't want it:

map.each |Obj v| { echo("v = $v") }

Value must be first in order to support that. I'm not sure that's a common enough case to warrant the abnormal ordering though. I lean towards changing it to key,value - and you'll have to specify a key arg even if you don't want it:

map.each |Str k, Obj v| { echo("v = $v") }

Yes, I think it's better. If you feel strongly about the use case where someone might only want the value, how about providing an each_value method?

Also, is there a significant performance penalty in providing two values to the closure instead of just one?

how about providing an each_value method?

This is actually what I suggested above. Map could have "each" "eachValue" and "eachKey" as three separate methods.

There isn't a significant cost in creating the closure, but it would require an extract object to be allocated every time. An object that would go unused. And that could create problems later on if Fan eventually gets a lint-like tool, which usually complain about unused variables.

I make it |V, K| to keep each consistent with List.each which passes the index as the optional second parameter. So the value is always passed first, and the key/index is always passed second (which you can optionally ignore).

That may be another place where it would be appropriate to split it. You could have List.each which iterates through items and List.enumerate (or something, enumerate is the Python term for it), which passes key, value (in the appropriate order of arguments).

It just feel a little too magical if you're allowed to "leave off" block parameters. But that's just my feeling towards things.

The way I think about it - leaving off block parameters is just the reverse of having default parameters when calling a method. It is nice way to create a concise API without a lot of duplicated functionality (which is why I don't like two versions of each for iteration).

I see how it's different from conventions elsewhere (and that can cause confusion), but I like that it's consistent with List.each.

If this was a debtate in Java I would have gone for eachKey, eachValue and eachEntry.

However, I think the duck-typing feature of Fan justifies the current decision of Map.each and List.each - it just makes life easier for abstraction and generalisation.

Having said that, I do have frequent encounters with Map where the keys have more meanings than its values - things like [Obj,Bool], WeakHashMap<T,T> etc.

Map.each(Value,Key) is just the wrong way around as programmer brains work - a map is a key->value pair, and that is the order that they should always appear in.

My proposal would be:

List.each(Value, Index)
Map.each(Key, Value, Index)
Map.eachValue(Value, Index)

where you could ignore/leave out all subsequent parameters. This means that the following work as expected:

List.each(Value)
Map.each(Key)
Map.each(Key, Value)
Map.eachValue(Value)

which seem like the natural method setup to me (lists are iterated by values by default, maps are iterated by keys by default, the index can always be queried).

BTW, I don't buy the "consistent with List" argument. If you want to model Lists in fan as Integer indexed Maps, then I would. But so long as they are two separate data structures, with different literals etc then they should behave as per their generally accepted contracts in other languages.

I changed my mind on this one. I agree with Cedric that each |value, key| is confusing. I think it should be each |value, index| even if the order isn't guaranteed. (And guaranteeing order, such as for LinkedHashMap in Java, would be interesting to consider but perhaps overkill. I think Python guarantees order in Dict objects these days.)

I also think there should be an eachPair |key, value| method.

I'll stick with my proposal. each(value, key) is just plain confusing. The methods need to focus on the common cases, which are iterate map by keys only, and iterate map by key and value. Iterate by values only is the rare case, and would thus get a differently named method eachValue(value).

And when providing an index into a map it doesn't matter that the index isn't consistent. Most people won't use the parameter, but when you do need it, its so much cleaner (like determining if its the first/last time around the loop)

I could live with your proposal, too. Sad to break the similarity, but your use cases really are nice.

I think it should be each |value, index| even if the order isn't guaranteed.

You are suggesting that Map.each pass the integer index instead of the key? I don't think that makes sense with the class taken as a whole. Remember List and Map are both keyed collections, it is just that List restricts the keys to a contiguous range of Ints. Because of this design, the two classes support duck typing:

collection->get(key)
collection->set(key, val)
collection->each |Obj val, Obj key| {...}

The code above works for both List and Map because they support the same duck typed interface.

I think each(value, key) is indeed going to confuse lots of developers. However, PHP takes this approach for for-each loops:

foreach ($array as $value) {
   // Looping over values
}

foreach ($array as $key => $value) {
   // Looping over key/value pairs
}

Likely because in PHP, a list is just an array whose keys are integer indices. Fan tries a similar approach, so maybe each(value, key) would work here, too.

The code above works for both List and Map because they support the same duck typed interface.

Thats an interesting point, but I think thats a bit esoteric (I think you'd be more likely to just call map.values if you're going to use it as a List).

In any event, I think this should suit the common case, were each should be Key,Value - it seems unnecessarily unorthodox at the moment.