golang / tour

[mirror] A Tour of Go
BSD 3-Clause "New" or "Revised" License
1.54k stars 521 forks source link

tour: [Confusing method name Abs in example] #616

Open scruffycatherder opened 6 years ago

scruffycatherder commented 6 years ago

Context: https://tour.golang.org/methods/1

Starting on slide 1 of the Methods tour, the examples frequently use a method:


func (v Vertex) Abs() float64 {
    return math.Sqrt(v.X*v.X + v.Y*v.Y)
}

Shouldn't this be called Hypotenuse() or Hypot()?
Abs() is typically understood as "absolute value". This discrepancy makes the examples in the tour very confusing.

In another example in the tour, there is a function Abs that actually implements absolute value...

Context: https://tour.golang.org/methods/3

func (f MyFloat) Abs() float64 {
    if f < 0 {
        return float64(-f)
    }
    return float64(f)
}

Then in the next slide, it goes back to an Abs method that implements hypotenuse instead Context : https://tour.golang.org/methods/4

func (v Vertex) Abs() float64 {
    return math.Sqrt(v.X*v.X + v.Y*v.Y)
}

This should also be named Hypotenuse.

There may be more examples of this misnomer. I suspect that someone got a bit overzealous with a refactoring and renamed some functions incorrectly.

ALTree commented 6 years ago

This has been brought up in the past. See my comment at https://github.com/golang/tour/issues/536#issuecomment-401058809.

scruffycatherder commented 6 years ago

Ahah, I see. In that case, perhaps rename Vertex to Vector, and do away with the previous references to Hypot()?

pinkgothic commented 3 years ago

Perhaps consider just give it a different name in both cases, because unfortunately, it really looks consistently wrong as it is now. Seconding the request to rename Vertex to Vector, I think that would help a bit. But even then I would rename the function, e.g. to something like Length() (not ideal as a shared function name, but I'd honestly prefer it), or really anything that doesn't carry the associative baggage that Abs() comes with. Other options that come to mind: Magnitude(), Distance(), DistanceToZero()... I'm not very creative here, sorry.

I think it's worth observing that this seems to crop up again every once in a while (and I was tempted to create a ticket, myself), so I think it's really strongly worth considering a change.