Multi pop/shift

[schwern]

Popping or shifting multiple items off an array is a pita. Either you have to figure out how to use splice to do it, or use a for loop.

Make a pop and shift that take the number of things to pop off.

Issue: pop() returns a scalar. @Array->pop(3); should return an array or array ref. Does this mean we modify @Array->pop so it returns an array if there's an argument? Or do we write popn? I lean towards popn/shiftn.

[kentfredric]

+1 for popn.

[player1537]

I am working on this task for Google Code-In. So far, in my version of popn(), given this code: my @array = (1, 2, 3, 4, 5); my @Newarray = @Array->popn(3); It will return @array == (1, 2); and @Newarray == (5, 4, 3). My question is, should it be (5, 4, 3) or (3, 4, 5)? My argument for (5, 4, 3) is, it is the same as @Newarray = (@Array->pop(), @Array->pop(), @Array->pop()); But the counter-argument is, you are asking for a lump of 3 things, and maybe you'd want to individually pop from that array (ie, @newarray->pop() would return what @Array->pop would, before the popn). What are others thoughts on this?

[schwern]

I've thought up some more arguments in favor of (3,4,5).

• Doing it the "in one lump" way is harder to do otherwise. splice @a, -$n, $n. Any time a user has to reach for splice they have to reach for the documentation to remember how it works.
• It's symmetrical with push. push @a, @a->popn; is identity.
• If you think of it as "give me the last N elements" it makes a lot more sense from a user's perspective. It's not often you need to pop the last N elements off a stack, but more often you want the last N elements of a list.

I think that last one is the killer. Perl programmers work with lists far more often than stacks.

@player1537 Unless there's further discussion, please do it the (3,4,5) way. That's basically splice @a, -$n, $n. There's also the accompanying shiftn where @a->shiftn(3) would produce (1,2,3).

Also note that all perl5i array operations return an array in list context and an array reference in array context. This allows them to chain like @a->popn(3)->sort->join(", ")->say. "Give me the last 3 elements, sort them, join them together with commas and print it". There's plenty of examples in lib/perl5i/2/ARRAY.pm using wantarray.

[player1537]

Okay, I've changed popn to return (3, 4, 5). Also, I just went with a loop instead of using splice or something similar, so... Should I change that to splice (maybe for speed issues?) or just leave it the way I thought to do it first? Also, I've already written tests for shiftn and popn, and I followed the wantarray pattern that all the other methods used :) When, or how, should I get the code to you? Or should I just wait until you get back on IRC?

[player1537]

I've re-written the code to use splice() and all of my tests still work. The ARRAY.pm diff, as well as t/popn.t and t/shiftn.t are up on my server, at: http://tanner.isthecoolest.com/filesys/popn-shiftn.tar Let me know if anything needs to be changed about it :)

[schwern]

@player1537 Good work. Thorough testing on the argument boundaries. I'm putting it into a branch called issue/149 so we can collaborate on it. If you git fetch, or reclone, you should have that branch as origin/issue/149.

Here's a few observations and changes.

• A negative value is an error. There's no useful meaning and we like to fail early.
• No argument could default to popping a single value... but then it's just pop and there's no reason to want to hard code that. So that should be an error, too.
• @a->popn(0) could be an error or it could return an empty list. There's arguments either way. On the one hand, popping nothing is a no-op so why would you do that? On the other hand, if you're doing things programmatically, like for my $things (@a->popn($i)) sometimes it's nice to not have to worry about the zero case. IMO I'm for popn(0) quietly doing nothing. It can return early in the code.

I'll make those changes.

• @a->popn($i) where $i is longer than the array (good catch in the tests) could do a few things...

1. It could be an error, like with splice, but I think that's obnoxious for similar reasons to why popn(0) shouldn't be an error.
2. It could quietly return the whole array, as you do now.
3. Or it could quietly return the whole array with the rest filled in with undefined values.
4. It could do 2 or 3 with a warning.

3 is interesting because it guarantees that the array returned will be the same length as what you asked for. @a->popn(10) will always return a list of 10 elements. It also doesn't mask the fact that you walked off the end of the array, because if you work with those elements some will be undefined... though the source of the undefined elements will be difficult to track down so I don't know how useful that really is.

4 is interesting because it still allows the operation and it marks the place where the problem happened.

I think it comes down to 2 or 4+3. Either it should shut up and just DWIM or it should warn immediately.

Thoughts?

[schwern]

@player1537 PS For this to be a complete patch it needs documentation. That goes into lib/perl5i.pm in the "Array Autoboxing" section. Documentation is written using Plain Old Documentation (see http://perldoc.perl.org/perlpod.html) and you can just crib from any of the other entries.

[player1537]

Indeed, I had thought about, and I think I even implemented it one time, making @Array->popn($too_long) return extra values. At first, I wanted to make it return 0s, except that the data type could be different from a number. I also didn't want to write a test for comparing against undefined values, so I chose not to fill it with those. I'm liking just returning the whole array.

For the part about an arg of 0, I chose to do that basically for the same reasons as you. I'll write some documentation for this when I get home, being sure to go over what the various arguments do. Should I make a case of string arguments? Right now, if you give it a string arg, it fails in the function, with some odd error (like, non-numeric comparison values for ">").

[player1537]

I wrote some tests, and fixed the functions to die on <0 || !defined, but what do you mean by "test in scalar contexts"? Once I get that scalar contexts part worked out, this should be finished :) Oh, and documentation, which I'll get started on now.

[schwern]

@player1537 "Test in scalar context" is checking that my $array_ref = @array->popn(3) works. That it returns an array reference. Everything perl5i tries to work like that. But I already wrote that test. See 40c264f

Erroring out nicely on string arguments would be good, yes. perl5i has methods you can use to check if a number is an integer or positive. See is_integer and is_positive. So $times->is_integer and ($times->is_positive or $times == 0) could be the whole check.

I'm not sure if you noticed that I've done work on top of your initial patch. That's what those "schwern referenced this issue from a commit" messages are for. It's in a new branch called issue/149. You should be working on top of that. Let me know if you don't know how to get it. http://help.github.com/remotes/ has some help on updating your repository.

A test against undefined values is fine. The testing functions all work pretty well. is_deeply [undef], [undef] will work.

[player1537]

Ah, that makes sense about the scalar context :) I'll try to change things over to use ->is_positive, etc. Regarding the new branch, I tried to download it, but... Ah, my issue was trying to clone everything in the same directory as I have been using. I see your commits, now. I'll go ahead and update it with the documentation I wrote and add in ->is_integer tests. Also, I don't know how to check in an update, so... I'll probably just tar the diffs and such, again.

[player1537]

Here is a tar of: t/popn.t, t/shiftn.t, lib/perl5i.pm, and lib/perl5i/2/ARRAY.pm. http://tanner.isthecoolest.com/filesys/issue.tar Hopefully, if you are ever online when I am, you can help me with committing things myself, so I don't have to do this again :)

The updates are: fixing popn and shiftn to error out on string args (or, rather, non-integer), adding that to the tests, and adding some documentation for popn and shiftn.

[schwern]

I'm happy with where it's at. Closing it up.