XYCursorChartNode code review and getDataExists bug

[pixelzoom]

Added to XYCursorPlot in by @jessegreenberg in 7fe14e2063bb6b3d3897b5794f012e211b00c276:

  /**
   * Returns true if any data is attached to this plot.
   * @returns {boolean}
   * @public
   */
  getDataExists() {
    let dataExists = false;
    this.valueSeriesListenerMap.forEach( ( listener, dataSeries, map ) => {
      dataExists = dataSeries.hasData();

      // break early
      if ( dataExists ) {
219     return;
      }
    } );

    return dataExists;
  }

This function returns boolean, and line 219 is violating that -- and probably returning falsy!

[pixelzoom]

Also note:

(1) Functions with multiple return points are (in general) an anti-pattern to be avoided.

(2) If you are going to use multiple return points, there's no need for dataExists:

  getDataExists() {
    this.valueSeriesListenerMap.forEach( ( listener, dataSeries, map ) => {
      if ( dataSeries.hasData() ) {
        return true;
      }
    }
    return false;
  }

[pixelzoom]

Since you're only looking at dataSeries (the keys), you should probably be iterating over this.valueSeriesListenerMap.keys, like (untested):

  getDataExists() {
    this.valueSeriesListenerMap.keys().forEach( dataSeries => {
      if ( dataSeries.hasData() ) {
        return true;
      }
    } );
    return false;
  }

Also...

• [x] The definition of this.valueSeriesListenerMap is insufficiently documented and would fail code review. For Maps, you should document the type of keys and values.

    // @private - Keep track of listeners for each series so the listeners can be removed when the series is removed
    this.valueSeriesListenerMap = new Map();

[pixelzoom]

Sorry to turn this into a code review, but I'm in here because of https://github.com/phetsims/fourier-making-waves/issues/3.

There is confusing inconsistency with how you refer to the Map keys. The name this.valueSeriesListenerMap leads me to believe that the keys are "valueSeries". They are also named as parameters dataSeries and dynamicSeries, and put in this.dynamicSeriesList. They are of type DynamicSeries, so dynamicSeries is correct and should be used throughout.

After I untangled that naming...

• [x] getDataExists doesn't involve the Map at all, and can be implemented as:

getDataExists() {
  return _.some( this.dynamicSeries.list, dynamicSeries => dynamicSeries.hasData() );
}

EDIT: ... and renamed to hasData.

[pixelzoom]

• [x] this.dynamicSeriesList = []; is missing visibility annotation. Based on current uses, it would be @protected -- only used in subclass EnergyPlot. But it's unclear whether you meant for it to be @public. In either case it should also be (read-only) because add/remove is handled by addDynamicSeries and removeDynamicSeries.

EDIT: I made it @protected (read-only) and renamed to dynamicSeriesArray (an array is not a list, they are different data structures).

[pixelzoom]

• [x] updateChartCursorPos is a non-standard abbreviation that fails code review. Rename to updateChartCursorPosition.

[pixelzoom]

• [x] Co-locate addDynamicSeries and removeDynamicSeries in the source file. Why are they separated by almost 100 lines?

[pixelzoom]

I've converted actionable things in the above comments to checklist items.

[pixelzoom]

Probably faster if I make these changes and have @jessegreenberg review. Self assigning.

[pixelzoom]

Co-locate addDynamicSeries and removeDynamicSeries in the source file. Why are they separated by almost 100 lines?

• [x] Same problem with cursor-related methods, e.g. setCursorValue, getCursorValue, etc. It's very helpful to the reader to group related functions.

[pixelzoom]

• [x] The implementation sometimes refers to "cursor" (e.g setCursorValue) and sometimes "chart cursor" (e.g updateChartCursorVisibility. Pick one and use it consistently.

[pixelzoom]

• [x] XYCursorPlot is accessing private fields of superclass ScrollingChartNode: plotWidth, plotHeight

• [x] ChartCursor is accessing private API of XYPlotCursor: plotWidth, plotHeight, minRecordedXValue, maxRecordedXValue

• [x] SeismographNode subclass is accessing private fields of ScrollingChartNode

[pixelzoom]

More...

• [x] ESP EnergyAccordionBox is accessing @protected field this.graphPanel of ScrollingChartNode

• [x] "graph" and "chart" are used interchangeably in APIs. Pick one and use it consistently. EDIT: I went with "chart".

[pixelzoom]

An issue, throughout griddle and spilling over into sims, is terminology. I see "plot", "chart", and "graph" being used interchangeably. A plot is not a chart. A plot displays a data set (aka data series). A chart displays one or more plots. Names like XYCursorPlot and ESP.EnergyPlot are incorrect and misleading - they are charts, not plots. I'll create a separate issue for terminology.

So... Stopping here with changes to XYCursorPlot. @jessegreenberg please review.

[jessegreenberg]

Thanks for all of this cleanup, changes are a big improvement. Since we were are discussing terminology in #71, I noticed a few things got changed from "plot" -> "chart", like chartWidth, chartHeight, chartPanel. But we decided that XYCursorPlotNode is a "plot" in #71. Should these all be renamed to plot for consistency?

[pixelzoom]

No, those names should definitely be "chart". See https://github.com/phetsims/griddle/issues/71#issuecomment-697106346.

[pixelzoom]

Note that XYCursorPlot is now named XYCursorChartNode.

[samreid]

It seems everything is checked off here except for things that were moved to other issues. I reviewed the commit messages and didn't see any trouble. Closing.