phetsims / dot

A math library with a focus on mutable and immutable linear algebra for 2D and 3D applications.
http://scenerystack.org/
MIT License
13 stars 6 forks source link

Vector2 and Vector2IO have duplicate toStateObject and fromStateObject methods #76

Closed zepumph closed 5 years ago

zepumph commented 6 years ago

Vector2 and Vecto2IO both have the same lines of code, we probably don't want that logic in both places, but it is hard to tell the usages of Vector2.toStateObject.

pixelzoom commented 6 years ago

They also have duplicate fromStateObject methods.

zepumph commented 6 years ago

From https://github.com/phetsims/graphing-quadratics/issues/38#issuecomment-426442151:

That looks good. I think that the level of verbosity is appropriate. We often to refer to these types of objects as "value types", but I think data structure is good too.

@samreid why are toStateObject and fromStateObject directly on the types for Vector2 and Quadratic. That feels like an antipattern to me, could we use phetsims/phet-io#1373 to make things more consistent?

Like vector2.phetioType.toStateObject()

pixelzoom commented 6 years ago

Defining toStateObject and fromStateObject directly on the wrapped type (e.g. Vector2, Color, Quadratic) is far from an anti pattern. It's the pattern called encapsulation. Rather, putting them on the IO type (e.g. Vector2IO, ColorIO, QuadraticIO) is the anti pattern; it requires exposing implementation details of the wrapped type to the IO type, duplication of documentation, information spread out across multiple files,... and an implementation that is inherently more problematic to maintain.

samreid commented 5 years ago

For Vector2 I agree it makes the most sense to move these methods from Vector2IO to Vector2. Leaving assigned to @zepumph for review/discussion. Anything else to do here?

zepumph commented 5 years ago

That looks good for Vector2. I wonder if we may use this same approach in the future for other value types like RangeIO or Bounds2IO.

zepumph commented 5 years ago

We don't have a need for that now though, closing.