code review 2192048: exp/draw: add Point.Eq, Point.Mul, Point.Div, Rectangle...

exp/draw: add Point.Eq, Point.Mul, Point.Div, Rectangle.Size methods

[rog, 2010-09-21 10:43:41]

Hello nigeltao (cc: golang-dev@googlegroups.com, r),

I'd like you to review this change.

[nigeltao, 2010-09-21 14:27:49]

I'm OK with Eq, but not sure about the usefulness of Mul and Div.

When I first saw the method named Size, I thought it calculated area. Perhaps
Dimension, Dim, or even just D might be a better name, analogous to Dx and Dy??

[rog, 2010-09-21 14:36:08]

On 21 September 2010 15:27,  <nigeltao@golang.org> wrote:
> I'm OK with Eq, but not sure about the usefulness of Mul and Div.

they're traditional. they were part of the original draw.Point method set.

and they're useful too. for instance, if you had Rectangle.Size,
the centre of a rectangle is given by
r.Min.Add(r.Size().Div(2))

> When I first saw the method named Size, I thought it calculated area.
> Perhaps Dimension, Dim, or even just D might be a better name, analogous
> to Dx and Dy??

i'm not sure i like any of those names. i always thought that the 'D'
in Dx and Dy stood
for Delta, but i don't like that too much as a name either either.

if there was to be a method for calculating area, surely it would be
named Area?

[nigeltao_gnome, 2010-09-21 14:48:59]

On 22 September 2010 00:36, roger peppe <rogpeppe@gmail.com> wrote:
> On 21 September 2010 15:27,  <nigeltao@golang.org> wrote:
>> I'm OK with Eq, but not sure about the usefulness of Mul and Div.
>
> they're traditional. they were part of the original draw.Point method set.

They were in the original draw.Point method set but I removed them
because they didn't appear to get a lot of use.


> and they're useful too. for instance, if you had Rectangle.Size,
> the centre of a rectangle is given by
> r.Min.Add(r.Size().Div(2))

To me, if I were to add a function or method to help calculate
centers, my instinct would be to add a midpoint function so it's
Midpoint(r.Min, r.Max). But calculating centers isn't that hard...


>> When I first saw the method named Size, I thought it calculated area.
>> Perhaps Dimension, Dim, or even just D might be a better name, analogous
>> to Dx and Dy??
>
> i'm not sure i like any of those names. i always thought that the 'D'
> in Dx and Dy stood
> for Delta, but i don't like that too much as a name either either.
>
> if there was to be a method for calculating area, surely it would be
> named Area?

Yeah, Size might end up being the right name for it. Just thinking out loud...

[rog, 2010-09-21 15:17:18]

On 21 September 2010 15:48, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 22 September 2010 00:36, roger peppe <rogpeppe@gmail.com> wrote:
>> On 21 September 2010 15:27,  <nigeltao@golang.org> wrote:
>>> I'm OK with Eq, but not sure about the usefulness of Mul and Div.
>>
>> they're traditional. they were part of the original draw.Point method set.
>
> They were in the original draw.Point method set but I removed them
> because they didn't appear to get a lot of use.

almost nothing gets a lot of use currently, as it's all experimental :-)

however, in a system with similar primitives, i've used both
mul and div a reasonable amount and found them useful.

a quick count gives me 21 occurrences in that code base.

obviously they are just a notational convenience, but they're
so simple that... why not?

some little idioms from those occurrences, translated to go:

from an implementation of Snake:
    // don't allow 180° turn.
    if !newdir.Eq(dir) && !newdir.Eq(dir.Mul(-1)) {

calculating the area of a deck of spread cards:
    return cr.Add(delta.Mul(r.start)).Union(cr.Add(delta.Mul(r.end - 1)))

another way of calculating the centre of a rectangle (assuming no overflow :-)):
    return r.Min.Add(r.Max).Div(2)

[rsc, 2010-09-21 19:36:17]

Mul and Div are less useful than Add and Sub
but complete a natural set and are useful.

In the entire Plan 9 distribution source tree, the
counts of number of lines containing each function
are 256 addpt, 139 subpt, 36 mulpt, and 31 divpt
(excluding tests).

Russ

[nigeltao, 2010-09-21 23:52:20]

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go
File src/pkg/image/geom.go (right):

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go#newcode31
src/pkg/image/geom.go:31: // Eq returns true if p and q are equal.
s/true if/whether/ would match Rectangle.Eq.

I'd also spread these one-liners over three lines. They're just as short as
other three-liners in this file.

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go#newcode34
src/pkg/image/geom.go:34: // Mul returns p scaled by k: Pt(p.X*k p.Y*k).
"Mul returns the vector p*k." would match the doc comments for Add and Sub.

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go#newcode181
src/pkg/image/geom.go:181: // Size returns the size of r as a Point. It is
equivalent to
How about replacing the two lines with "// Size returns r's width and height."?

Also, move this up to line 70ish near the Dx and Dy methods.

[rog, 2010-09-22 10:46:13]

PTAL

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go
File src/pkg/image/geom.go (right):

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go#newcode31
src/pkg/image/geom.go:31: // Eq returns true if p and q are equal.
On 2010/09/21 23:52:20, nigeltao_golang wrote:
> s/true if/whether/ would match Rectangle.Eq.
> 
> I'd also spread these one-liners over three lines. They're just as short as
> other three-liners in this file.

Done.

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go#newcode34
src/pkg/image/geom.go:34: // Mul returns p scaled by k: Pt(p.X*k p.Y*k).
On 2010/09/21 23:52:20, nigeltao_golang wrote:
> "Mul returns the vector p*k." would match the doc comments for Add and Sub.

Done.

http://codereview.appspot.com/2192048/diff/6001/src/pkg/image/geom.go#newcode181
src/pkg/image/geom.go:181: // Size returns the size of r as a Point. It is
equivalent to
On 2010/09/21 23:52:20, nigeltao_golang wrote:
> How about replacing the two lines with "// Size returns r's width and
height."?
> 
> Also, move this up to line 70ish near the Dx and Dy methods.

Done.

[nigeltao_gnome, 2010-09-23 00:58:17]

LGTM.

[nigeltao, 2010-09-23 00:59:03]

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

exp/draw: add Point.Eq, Point.Mul, Point.Div, Rectangle.Size methods

R=nigeltao, nigeltao_golang, rsc
CC=golang-dev, r
http://codereview.appspot.com/2192048

Committer: Nigel Tao <nigeltao@golang.org>