code review 160064: Add Count, Cycle, ZipWith, GroupBy, Repeat, RepeatTimes...

Add Count, Cycle, ZipWith, GroupBy, Repeat, RepeatTimes, Unique to exp/iterable.

Modify iterFunc to take chan<- instead of just chan.

[sigpipe, 2009-11-25 04:25:07]

Hello rsc, dsymonds1,

I'd like you to review the following change.

[dsymonds, 2009-11-25 04:51:39]

http://codereview.appspot.com/160064/diff/1001/1002
File src/pkg/exp/iterable/iterable.go (right):

http://codereview.appspot.com/160064/diff/1001/1002#newcode214
src/pkg/exp/iterable/iterable.go:214: // Count returns consecutive integers
starting with n.
This seems like it could be generalised, and perhaps should not be a specific
function itself. What about just a simple NaturalNumbers type to do this? It's
what integerStream is in the test file, and users could just use Drop to skip
the first few. Alternatively, expand integerStream to do this.

http://codereview.appspot.com/160064/diff/1001/1002#newcode235
src/pkg/exp/iterable/iterable.go:235: func Chain(args ...) Iterable {
How much of a loss would it be if the argument was []Iterable instead? That
would simplify this function's code and make it catch type errors at compile
time.

http://codereview.appspot.com/160064/diff/1001/1002#newcode251
src/pkg/exp/iterable/iterable.go:251: func ZipWith(f interface{}, args ...)
Iterable {
Again, could this arg be []Iterable?

Also, this one feels like it could just be an adapter to be used with a generic
Zip implementation.

http://codereview.appspot.com/160064/diff/1001/1002#newcode310
src/pkg/exp/iterable/iterable.go:310: // Slice returns an Iterable that contains
elements in the range specified offset from the head of the input Iterable.
Can Take be rewritten to call this?

http://codereview.appspot.com/160064/diff/1001/1002#newcode339
src/pkg/exp/iterable/iterable.go:339: // RepeatTimes generates a stream of n
copies of v.
Slice + Repeat?

[sigpipe, 2009-11-25 06:01:44]

PTAL.

On 2009/11/25 04:51:39, dsymonds1 wrote:
> http://codereview.appspot.com/160064/diff/1001/1002#newcode214
> src/pkg/exp/iterable/iterable.go:214: // Count returns consecutive integers
> starting with n.
> This seems like it could be generalised, and perhaps should not be a specific
> function itself. What about just a simple NaturalNumbers type to do this? It's
> what integerStream is in the test file, and users could just use Drop to skip
> the first few. Alternatively, expand integerStream to do this.

I think this sounds like a good idea.  I will play around with it a bit.

> http://codereview.appspot.com/160064/diff/1001/1002#newcode235
> src/pkg/exp/iterable/iterable.go:235: func Chain(args ...) Iterable {
> How much of a loss would it be if the argument was []Iterable instead? That
> would simplify this function's code and make it catch type errors at compile
> time.

Updated.  This works great.

> http://codereview.appspot.com/160064/diff/1001/1002#newcode251
> src/pkg/exp/iterable/iterable.go:251: func ZipWith(f interface{}, args ...)
> Iterable {
> Again, could this arg be []Iterable?

Ditto.

> Also, this one feels like it could just be an adapter to be used with a
generic
> Zip implementation.

Yes.  I broke the function up into Zip() and Apply() and just made ZipWith() a
Map() operation.

> http://codereview.appspot.com/160064/diff/1001/1002#newcode310
> src/pkg/exp/iterable/iterable.go:310: // Slice returns an Iterable that
contains
> elements in the range specified offset from the head of the input Iterable.
> Can Take be rewritten to call this?

Done.

> http://codereview.appspot.com/160064/diff/1001/1002#newcode339
> src/pkg/exp/iterable/iterable.go:339: // RepeatTimes generates a stream of n
> copies of v.
> Slice + Repeat?

Done.

I also rewrote Unique() to Map() over GroupBy() instead of an explicit loop.

[dsymonds, 2009-11-25 23:18:53]

http://codereview.appspot.com/160064/diff/8/9
File src/pkg/exp/iterable/iterable.go (right):

http://codereview.appspot.com/160064/diff/8/9#newcode149
src/pkg/exp/iterable/iterable.go:149: func Take(iter Iterable, n int) Iterable	{
return Slice(iter, 0, n) }
Nice.

http://codereview.appspot.com/160064/diff/8/9#newcode228
src/pkg/exp/iterable/iterable.go:228: // Zip returns a stream of []interface{}
consisting of the next element from each input Iterable.
s/a stream/an Iterable/

http://codereview.appspot.com/160064/diff/8/9#newcode257
src/pkg/exp/iterable/iterable.go:257: fv, ok :=
reflect.NewValue(f).(*reflect.FuncValue);
It feels like a shame to bring in the reflect package to this package that is
otherwise only dependent on some container packages.

How bad would it make this to do the same transformation with f as with args? If
f is a func([]interface{}) then the caller can unpack it however they want
easily enough.

http://codereview.appspot.com/160064/diff/8/9#newcode327
src/pkg/exp/iterable/iterable.go:327: return iterFunc(func(ch chan<-
interface{}) {
This is equivalent to this, right?
  return Cycle(iterFunc(func(ch chan<- interface{}) {
    ch <- v
  }

Probably unnecessary, though.

http://codereview.appspot.com/160064/diff/8/9#newcode360
src/pkg/exp/iterable/iterable.go:360: if !reflect.DeepEqual(curkey, kv) {
This is another unfortunate reflect dependency. How would you feel about passing
in a cmp function (takes two interface{}, returns bool)?

[sigpipe, 2009-11-26 04:54:15]

On 2009/11/25 23:18:53, dsymonds1 wrote:
> http://codereview.appspot.com/160064/diff/8/9#newcode257
> src/pkg/exp/iterable/iterable.go:257: fv, ok :=
> reflect.NewValue(f).(*reflect.FuncValue);
> It feels like a shame to bring in the reflect package to this package that is
> otherwise only dependent on some container packages.
> 
> How bad would it make this to do the same transformation with f as with args?
If
> f is a func([]interface{}) then the caller can unpack it however they want
> easily enough.

Without using reflection, the better way then is to just provide a ZipWith and
ZipWith3 that take functions with 2 or 3 arguments, respectively.  This makes
the user code uglier (see iterable_test.go) but you do get compile time errors
instead of runtime.

Disadvantges:
- user function can only return a single value
- user functions must take interface{} as args and use lots of type assertions

> http://codereview.appspot.com/160064/diff/8/9#newcode360
> src/pkg/exp/iterable/iterable.go:360: if !reflect.DeepEqual(curkey, kv) {
> This is another unfortunate reflect dependency. How would you feel about
passing
> in a cmp function (takes two interface{}, returns bool)?

I chose an alternative, which is to create a Key interface that provides two
functions: Compute and Equal.

This also has the unfortunate effect of creating lots of boilerplate in the user
code since it will be necessary to provide identity functions for each type in
order to use the Unique() function.

Other changes:
- Removed Count()

[dsymonds, 2009-11-26 23:40:50]

LGTM

Looks fine overall. Over to rsc.

http://codereview.appspot.com/160064/diff/2002/2003
File src/pkg/exp/iterable/iterable.go (right):

http://codereview.appspot.com/160064/diff/2002/2003#newcode218
src/pkg/exp/iterable/iterable.go:218: // Zip returns an Iterable of
[]interface{} consisting of the next element from each input Iterable.
Add a comment about the behaviour with Iterables of different lengths.

http://codereview.appspot.com/160064/diff/2002/2003#newcode244
src/pkg/exp/iterable/iterable.go:244: func ZipWith(f func(c, d interface{})
interface{}, a, b Iterable) Iterable {
For consistency I think this should be ZipWith2.

http://codereview.appspot.com/160064/diff/2002/2003#newcode299
src/pkg/exp/iterable/iterable.go:299: // Key defines the interface required by
the GroupBy function.
This isn't really a "Key", per se. I can't think of a better name off-hand
(perhaps rsc will have a good idea).

I'm not sure an interface is all that useful here, either. What kind of objects
would implement these stateless methods? What about just a struct here, since
it's so easy to write struct literals in Go?

[rsc, 2009-11-30 00:14:12]

A few comments.

http://codereview.appspot.com/160064/diff/2002/2003
File src/pkg/exp/iterable/iterable.go (right):

http://codereview.appspot.com/160064/diff/2002/2003#newcode206
src/pkg/exp/iterable/iterable.go:206: // Chain returns an Iterable that
concatentates all values from the specified Iterables.
Is there a reason this is called Chain instead of Concat?
(Is Chain a standard name?)

http://codereview.appspot.com/160064/diff/2002/2003#newcode222
src/pkg/exp/iterable/iterable.go:222: // handle case where there are zero args
comment only restates the next line (if len(args) == 0).
can delete comment.

http://codereview.appspot.com/160064/diff/2002/2003#newcode243
src/pkg/exp/iterable/iterable.go:243: // ZipWith returns an Iterable containing
the result of executing f using arguments read from a and b.
Maybe ZipWith and ZipWith3 should be rolled into a single

func ZipWith(f func([]interface{}), []Iterable) Iterable

?

http://codereview.appspot.com/160064/diff/2002/2003#newcode259
src/pkg/exp/iterable/iterable.go:259: // Slice returns an Iterable that contains
elements in the range specified offset from the head of the input Iterable.
// Slice returns an Iterable that contains the elements from iter
// with indexes in [start, stop).

http://codereview.appspot.com/160064/diff/2002/2003#newcode263
src/pkg/exp/iterable/iterable.go:263: if start < 0 || stop < 0 || stop <= start
{	// require a positive, non-zero distance
I think this can go away.
The tests below work without it,
and it allows writing

Slice(iter, 100, 100)

to get an iterable that closes only
after the original iterable has produced
100 elements.

http://codereview.appspot.com/160064/diff/2002/2003#newcode290
src/pkg/exp/iterable/iterable.go:290: return Slice(Repeat(v), 0, n)
Using Repeat this way generates a garbage goroutine
that will never be collected.  It would probably be better
to spend the extra two lines of code and write the loop.

http://codereview.appspot.com/160064/diff/2002/2003#newcode300
src/pkg/exp/iterable/iterable.go:300: type Key interface {
type Grouper interface {

http://codereview.appspot.com/160064/diff/2002/2003#newcode302
src/pkg/exp/iterable/iterable.go:302: Compute(interface{}) interface{};
s/Compute/Key/

[dsymonds, 2009-11-30 00:54:29]

http://codereview.appspot.com/160064/diff/2002/2003
File src/pkg/exp/iterable/iterable.go (right):

http://codereview.appspot.com/160064/diff/2002/2003#newcode206
src/pkg/exp/iterable/iterable.go:206: // Chain returns an Iterable that
concatentates all values from the specified Iterables.
On 2009/11/30 00:14:12, rsc wrote:
> Is there a reason this is called Chain instead of Concat?
> (Is Chain a standard name?)

Chain is pretty standard (think function chaining).

http://codereview.appspot.com/160064/diff/2002/2003#newcode300
src/pkg/exp/iterable/iterable.go:300: type Key interface {
On 2009/11/30 00:14:12, rsc wrote:
> type Grouper interface {

That's a better name, but I still think just passing in two functions (or a
struct with two function fields) would be simpler.

[rsc, 2009-11-30 01:07:54]

>> type Grouper interface {
>
> That's a better name, but I still think just passing in two functions
> (or a struct with two function fields) would be simpler.

Let's leave it at Grouper for now and see.
Passing in multiple functions gets heavy pretty easily.

[r, 2009-12-02 04:34:26]

is this CL active?

[sigpipe, 2009-12-02 04:36:45]

On 2009/11/30 00:14:12, rsc wrote:
> http://codereview.appspot.com/160064/diff/2002/2003#newcode243
> src/pkg/exp/iterable/iterable.go:243: // ZipWith returns an Iterable
containing
> the result of executing f using arguments read from a and b.
> Maybe ZipWith and ZipWith3 should be rolled into a single
> 
> func ZipWith(f func([]interface{}), []Iterable) Iterable
> 
> ?

Dave also made this suggestion.  However, I prefer having separate functions for
the 2- and 3-argument cases so that a compiler error is generated if the passed
function does not match the number of Iterables.  Otherwise it has to be a
runtime error.

I updated the patch with all the other suggestions.  Awaiting final word on the
ZipWith issue.

me

[sigpipe, 2009-12-02 04:37:23]

On 2009/12/02 04:34:26, r wrote:
> is this CL active?

Heh, missed it by >< that much.

[rsc, 2009-12-03 21:00:18]

looks good to me.
please make the one fix below 
and hg upload
(and let us know)
and i will submit it.

http://codereview.appspot.com/160064/diff/4001/4002
File src/pkg/exp/iterable/iterable.go (right):

http://codereview.appspot.com/160064/diff/4001/4002#newcode219
src/pkg/exp/iterable/iterable.go:219: // Terminates when the end of the shortest
Iterable is reached.
// The length of the returned Iterable is the minimum
// of the lengths of the input Iterables.

[sigpipe, 2009-12-04 02:43:43]

Updated comment for Zip.  PTAL.

[rsc, 2009-12-04 03:57:41]

LGTM

thanks

[rsc, 2009-12-04 04:03:10]

*** Submitted as http://code.google.com/p/go/source/detail?r=db8f28e4e8ac ***

Add Count, Cycle, ZipWith, GroupBy, Repeat, RepeatTimes, Unique to exp/iterable.

Modify iterFunc to take chan<- instead of just chan.

R=rsc, dsymonds1
CC=golang-dev, r
http://codereview.appspot.com/160064

Committer: Russ Cox <rsc@golang.org>