Intention for undocumented method (in class `Collection`) `lastForWhich` and `lastIndexForWhich`

Questions
1

When I am writing the SCLang polyfill for the JavaScript Array, I found two method lastForWhich and lastIndexForWhich that is categorised in undocumented method.

I have searched the repository and the discussion here, but I cannot find discussion or explicit call in code, for these two methods. (I also checked GitHub Wiki page, but I am afraid that the " mailing list archives" link might be broken and now it is showing 404).

For my guess, these two methods might be a doing-reversely version of Collection::detect (although the naming style seems not match). However, as the source code suggests, it only checks the first element of the array. The quickest fix to it is just delete the else clause if my guess is correct.

Collection
{
    // other code ...
	lastForWhich { | function |
		var prev;
		this.do {|elem, i|
			if (function.value(elem, i)) {
				prev = elem;
			}{
				^prev
			}
		};
		^prev
	}
	lastIndexForWhich { | function |
		var prev;
		this.do {|elem, i|
			if (function.value(elem, i)) {
				prev = i;
			}{
				^prev
			}
		};
		^prev
	}
}

Also, if my guess of the functionality is correct, I may want to suggest changing the naming of these two methods. Or maybe simply rename it like detectLast (which might be more intuitive).

2

I agree that the behavior doesn’t match the name. If there is at least one match, it will scan through the first matching items, but then stop immediately upon a non-match; if there is a match after that, it won’t be considered.

I wonder when it was added. There was a period for awhile when a lot of things went in without sufficient review or testing; this kinda smells like one of those. (No access to github on this phone, so I can’t check. I don’t think this is my addition, but if it is, I’ll eat my own medicine.)

In any case, I think it should be the same as detect, except using reverseDo instead of do (more efficient to search from the back and leverage early exit from the loop, rather than always scanning the entire array).

hjh

1 Like
4

Thank you for your timely and detailed description.

I wonder if it is still worth it for updating the bugfix by pull request, or making it work better by reverseDo ? If it will help, I am happy to fix it and provide some documentation :slightly_smiling_face:.

Also, can I ask will it be better to fix it as new method named like detectLast, and deprecate these two old method ?

(deleted my previous reply, since it did not seem to reply you, but myself)

5

Let’s see if anybody else has an opinion, but my vote would be for the reverseDo approach.

Forward search for the last match must touch every collection item.

Reverse search for the last match might only touch 1 or 2 items (but worst case – no match – still has to touch every item).

So it would be better not to require a full scan every time.

hjh

1 Like
6

Thank you for your reply. Let us wait for few days for other’s idea, also :grinning:.

Also, do you think it is good for me to open a topic at Development category, to ask if everyone agree with adding a reverse version for some functions ?
(Since I am new to SuperCollider, I do not know if SCLang values more about concise of class’s method, or more variaty of handy method)

Changes now can be viewed here.

7

Thanks for this!

Agree reverseDo would be an improvement.

I agree the names should be consistent if possible so maybe detectLast or detectr (to correspond to injectr) and detectLastIndex or detectrIndex.

1 Like
8

I just found that reverseDo’s parameter index for the function passed to it, is the index of reversed array (such as arr.reverse), but not the arr itself. Like the example of official document:

['a', 'b', 'c'].reverseDo({ arg item, i; [i, item].postln; });
// Prints: 
// [ 0, c ]
// [ 1, b ]
// [ 2, a ]
// But not: [2, c], [1, b], [0, a].

As source code suggests:

ArrayedCollection : SequenceableCollection {
	// Other code
	reverseDo { arg function;
		// special byte codes inserted by compiler for this method
		var i=0, j=0;
		i = this.size - 1;
		while ({ i >= 0 },{
			function.value(this.at(i), j); // Question: why `j` here, but not `i` ?
			i = i - 1;
			j = j + 1;
		})
	}
	// Other code
}

Can I ask the j passed as index here, is it by-design, or ? If so, I may need to change my commit posted before (since it directly returns, but not doing this.size - 1).

Looking forward for your reply :grinning: .

9 
+ SequenceableCollection {
	lastForWhich { |func|
		this.reverseDo { |item, i|
			if(func.value(item, i)) {
				^item
			}
		}
		^nil
	}
	lastIndexForWhich { |func|
		this.reverseDo { |item, i|
			if(func.value(item, i)) {
				^this.size - i - 1
			}
		};
		^nil
	}

}

hjh

1 Like
10

Thank you for your reply ! Indeed I should do like this.

11

Since there is no more replies appear, the pull request was made, and waiting for reviews.

Thank you for all who replies to this post, and gives me so many useful information :grinning:.