Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uniform access #98

Open
samselikoff opened this issue May 13, 2014 · 2 comments
Open

Uniform access #98

samselikoff opened this issue May 13, 2014 · 2 comments

Comments

@samselikoff
Copy link

The problem

I'm running into some issues with uniform access, and am wondering if there's a place on d3.chart's main chart object for get and set methods. This is more a discussion than a question.

Let's say I have a core-line-chart, and by default it sets an internal property _duration equal to 300:

// core-line-chart.js
initialize: function(options) {
  this._duration = 300;
}

Throughout its code it can access this property using this._duration. Now let's say we want this property to be configurable from the outside. We add a getter/setter:

  // core-line-chart.js
  initialize: function(options) {
    this._duration = 300;
  }
+ ...
+ duration: function(_) {
+   if (arguments.length === 0) {return this._duration;}
+
+   this._duration = _;
+
+   return this;
+  }

Now outside users can set .duration(500), and all the code in the chart that uses this._duration will work as expected.

Now, say that we also want other d3.charts that attach our core line chart to be able to modify its duration. There are two approaches. First, they could just use the core line chart's existing public interface:

// some-other-chart.js
initialize: function() {
  ...
  this.lineChart = this.base.append('g')
    .chart('core-line-chart')
    .duration(500);
  this.attach('line-chart', this.lineChart);
}

Alternatively, we could alter our core line chart to override its defaults on initialization:

  // core-line-chart.js
  initialize: function(options) {
-   this._duration = 300;
+   this._duration = options.duration || 300;
  }

Now the parent (attacher) chart can specify an optional duration when it attaches the line chart.

All this is fine, and everything still works as expected. The tricky part is when property values need to be determined at run time.

Let's say the parent chart's duration property is dynamic, and can itself be changed by a getter/setter. Now, even though it may change later, the core-line-chart's duration property is set as soon as it's attached, so it may fall out of sync. One solution is for the parent chart to, in its getter/setter, update the attached's charts property:

  // some-other-chart.js
  initialize: function() {
    ...
    this.lineChart = this.base.append('g')
      .chart('core-line-chart')
-     .duration(500);
+     .duration(this._duration);
    this.attach('line-chart', this.lineChart);
  },

  ...

  duration: function(_) {
    if (arguments.length === 0) {return this._duration;}

    this._duration = _;
+   this.lineChart.duration(_);

    return this;
  }

But this method is cumbersome, can quickly become unwieldy with lots of dependent properties, and is even more difficult when the parent is a subclass and the accessor lives in the superclass. We could also use pub/sub, but this suffers from the same problems.

Our child (attached) chart could also call the parent's accessor method directly (this._parent.duration()), but this violates the Law of Demeter and makes our child chart less configurable (the child chart has now made assumptions about which duration to use).

Another approach is for our parent chart to simply pass in its accessor function to the child chart:

  // some-other-chart.js
  initialize: function() {
    ...
    this.lineChart = this.base.append('g')
      .chart('core-line-chart')
-     .duration(this._duration);
+     .duration(this.duration);
    this.attach('line-chart', this.lineChart);
  }

Then, we'd alter our core-line-chart a bit, so that it works whether duration is a static property or a function

// core-line-chart.js
duration: function(_) {
  if (arguments.length === 0) {
    return (typeof this._duration === 'function')
      ? this._duration()
      : this._duration;
  }

  this._duration = _;

  return this;
},

and change our property access throughout the code from this._duration to this.duration(). Now, whenever our core-line-chart needs the duration, it will return either the static value, or it will invoke the getter method on the parent chart to dynamically compute the value.

This is certainly a solution, though using this.duration() to get a static value of 300 seems to go against the grain. We could simply use this._property for values we know are static and this.property() for calculated values, but this violates uniform access and requires a heavier cognitive load when composing and modifying charts.


Discussion

More importantly though, every user of d3.chart will run into these problems at some point, and have to go through a similar process to come up with a solution that takes care of all these cases.

One way we could prevent users from having to answer these questions themselves would be to add a .get and .set method to the core chart class. The .get method would essentially do the same thing as the last code snippet above: inspect the internal property; if it's a function then invoke it; otherwise, return the value. (Some properties like scales are both functions and objects, so in those cases we'd want to return the object).

This would mean throughout our charts we would use

this.get('duration')
this.get('xScale')

and so on, and reliably get the desired property whether it's a static default, a static property from the parent, or a dynamic property from the parent.

The framework could also endorse one way for parent charts to configure child charts. I'm not entirely sure this would work, but perhaps the default could be that properties passed in the options object could automatically be set on the child chart instance, so the child could access them via .get without any code. We could also allow users to redefine set if they needed to do validation/other work.

I'm interested to hear others' thoughts and experiences. If there's a simple solution to all this that I'm missing, by all means please share. d3.chart (like d3) gives us a lot of flexibility to build our charts as we see fit; but if we are all solving these problems in only trivially different ways, perhaps the framework can help push us in the right - and same - direction.

@g00fy-
Copy link

g00fy- commented Sep 11, 2014

I found myself also struggling with this problem. I ended up throwing away all my chart-components (axis, points etc - which also generated a lot of unneeded g wrappers) and using what I call features in one chart.

A feature is just a layer which can be enabled and disabled (lines, points, axis, grid etc..). Since feature is a part of the main chart it knows how to communicate with the chart and how to access and mutate it's properties - but still using uniform access. This pattern is less reusable but I found it more maintainable.

The only place where I found composition of charts useful was applying margins.
The chart which handles all logic is not aware of margins and dimensions. Those are applied by the parent chart which takes care of properly resizing the child chart and margin conventions. This also allows the parent chart to be composed of LegendChart and AxisChart.

@samselikoff
Copy link
Author

My thinking has led me to a similar area. Would love to see our implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants