Create a small data class for the return type of Optic.get*Shapes()

[zepumph]

I think it would be a bit more clear if instead of returning an object literal of shape keys, there was a private class that was a data type that stored those fields, and then the return type could be that class.

Something like OpticShapeCollection or OpticShapeStorage or ShapesForOptic. Up to you if you want to do this.

[zepumph]

#154

[zepumph]

Up to you if you want to do this.

Actually, now that I see

geometric-optics/js/common/model/Optic.js

Lines 93 to 101 in 0f9ba7d

	// @public {Property.<Object>} shapes (fill and outline) of the optical element
	this.shapesProperty = new DerivedProperty( [
	this.radiusOfCurvatureProperty,
	this.diameterProperty,
	this.curveProperty ],
	( radius, diameter, curve ) => {
	return this.getShapes( radius, diameter, curve );
	} );

, I would say definitely do this IMO.

[veillette]

I agree.

[veillette]

I created an OpticShapeCollection class that hosts all the shapes for rendering and ray hitting. In the process, I realized that the middleShape was unused and was therefore removed (see #171).

Overall, that is a nice improvement, that simplifies the optic class.
Assigning back to @zepumph for review.

[zepumph]

This is really excellent. I wasn't even thinking of being able to factor out the shape creation functions. This is a great divide of responsibility! Nice work.

• I would recommend setting all instance variables to null in the constructor, and documenting which of them may actually be set to null (not have a shape) after construction, and for which optic type that may occur.
• Optic.shapesProperty could be optionally renamed to Optic.shapeCollectionProperty. A bit clearer to go with the rename.

Thanks!

[pixelzoom]

Looks like these still a bit of work to be done here, as suggested in #167 (comment). So self assigning.

[pixelzoom]

All recommendations have been implemented. Closing.