3
\$\begingroup\$

In my data there can be different number of bars in each chart. Here is a picture:

vertical bar charts

When pressing the button labeled change bars layout the view of the charts toggles between horizontal and vertical bars. For example, like this:

horizontal bar charts

Since I might get bar chart stacked vertically or horizontally and there may be a different number of stacks I wasn't able use the D3 stack function and had to do most of the calculation myself.

2 more important points:

  1. my code is inside VueJS framework.

  2. the variable stackedXorY is 1 or 0, when it's 1 the stack is horizontal when it's 0 the stack is vertical.

A couple of questions:

  1. Is there a way to use the d3 stack or some other method to create this kind of chart?

  2. I noticed that when I update the chart (in handleChartLayout function) each selected stage holds the y property, even though the data itself (tasks) and stages in it don't hold it. How is that possible? It seems to me I have an issue but not sure.

Here is the an example of the data:

{  
   "id":"ACra92XfQ696H7BUCCYT",
   "isTaskFinished":true,
   "taskName":"t1",
   "initTaskTime":1536427448023,
   "endTaskTime":1536427471408,
   "taskStages":[  
      {  
         "initTime":1536427447023,
         "timeStamps":[  
            1536427449994,
            1536427453242,
            1536427456115
         ],
         "totalTime":12503
      },
      {  
         "initTime":1536427460526,
         "timeStamps":[  
            1536427465433,
            1536427470250
         ],
         "totalTime":10882
      }
   ],
   "taskTotalTime":23385
}

Here is the code:

    <template>
  <div>
    <svg
      ref="stacked-svg-tasks"
      id="stacked-svg-tasks"
      :width="svgWidth"
      :height="svgHeight"
      ></svg>
  </div>
</template>
<script>
import * as d3 from 'd3';

export default {
  name: 'TasksStackedBarChart',
  props: ['tasks', 'svgWidth', 'svgHeight', 'stackedXorY'],
  data() {
    return {
      X_AXIS_HEIGHT: 20,
      Y_AXIS_WIDTH: 50,
      svgMargin: { top: 20 },
      barChartGroup: {
        width: 0,
        height: 0,
        x: 0,
        y: 0,
      },
      GbT: 50, // gap between tasks
      stageWidth: 0,
      toalStages: 0,
      xPosOfTasks: [],
      yScale: null,
      yScaleDomainEnd: 0,
    };
  },

  created() {
    this.barChartGroup.width = this.svgWidth - this.Y_AXIS_WIDTH;
    this.barChartGroup.height = this.svgHeight - this.X_AXIS_HEIGHT;
    this.barChartGroup.x = this.Y_AXIS_WIDTH;
    this.barChartGroup.y = this.X_AXIS_HEIGHT;

    this.toalStages = this.tasks.map(v => v.taskStages.length).reduce((s, v) => s + v, 0);
    this.stageWidth = this.getStageWidth();
    this.xPosOfTasks = this.getXPosOfTask(this.GbT, this.tasks, this.stageWidth, this.stackedXorY);
    this.yScaleDomainEnd = this.getLongestTask();
    this.yScale = this.getScaleForTask(this.barChartGroup.height, this.yScaleDomainEnd);
  },

  mounted() {
    this.chartBuilder();
  },

  methods: {
    getScaleForTask(rangeEnd, domainEnd) {
      const calcRangeEnd = rangeEnd - this.svgMargin.top - this.X_AXIS_HEIGHT;
      return d3.scaleLinear()
        .range([0, calcRangeEnd])
        .domain([0, domainEnd]);
    },

    getLongestTask() {
      return d3.max(this.tasks.map(t => t.taskTotalTime));
    },

    getStageWidth() { // will keep using this. to see if it's better to work like this or i should pass parameters
      const numOfChartBars = this.stackedXorY ? this.toalStages : this.tasks.length;
      return Math.floor((this.svgWidth - (this.GbT * (this.tasks.length + 1))) / numOfChartBars);
    },

    getPosOfStage(stage, stageIndex) {
      const x = stageIndex * this.stageWidth * this.stackedXorY;
      // this sets the bars at the bottom than this.yScale(s.y) moves them accordingly
      const y = this.barChartGroup.height - this.yScale(stage.totalTime) - ((1 - this.stackedXorY) * this.yScale(stage.y));
      return `translate(${x}, ${y})`;
    },

    getXPosOfTask(gapBetweenTasks, tasks, widthOfBar, stackedYorX) {
      const newXPosOfTasks = [0]; // [gapBetweenTasks + 50];

      for (let c = 0; c < tasks.length - 1; c += 1) {
        const barsInTask = stackedYorX ? tasks[c].taskStages.length : 1;
        const taskXPos = gapBetweenTasks + (barsInTask * widthOfBar) + newXPosOfTasks[c];
        newXPosOfTasks.push(taskXPos);
      }
      return newXPosOfTasks;
    },

    cumulativeTimeOfStages(taskStages) {
      return taskStages.reduce((sums, curItem) => {
        const newSum = sums[sums.length - 1] + curItem.totalTime;
        sums.push(newSum);
        return sums;
      }, [0]);
    },

    reconstructedStageData(taskStages) {
      const cumulativeSums = this.cumulativeTimeOfStages(taskStages);
      return taskStages.map((stage, i) => ({ y: cumulativeSums[i], ...stage }));
    },

    chartBuilder() {
      // clearing svg
      d3.selectAll('#stacked-svg-tasks').selectAll('g').remove();

      const svg = d3.select('#stacked-svg-tasks');
      // adding Y axis
      const yAxisScale = d3
        .scaleTime()
        .domain([this.yScaleDomainEnd, 0])
        .range([0, this.svgHeight - this.X_AXIS_HEIGHT - this.svgMargin.top]);
      const yAxis = d3.axisLeft().scale(yAxisScale);

      const barChartGroup = svg
        .selectAll('g')
        .data([this.tasks])
        .enter()
        .append('g')
        .attr('class', 'bar-chart-group')
        .attr('transform', `translate(${this.barChartGroup.x},${this.barChartGroup.y})`);

      const taskGroups = barChartGroup
        .selectAll('g')
        .data(t => t)
        .enter()
        .append('g')
        .attr('class', (t, i) => `bar${i}`)
        .attr('transform', (t, i) => `translate(${this.xPosOfTasks[i]},0)`);

      const stageGroups = taskGroups
        .selectAll('g')
        .data(t => this.reconstructedStageData(t.taskStages))
        .enter()
        .append('g')
        .attr('transform', (s, i) => this.getPosOfStage(s, i))
        .append('rect')
        .attr('width', this.stageWidth)
        .attr('height', s => this.yScale(s.totalTime))
        .attr('fill', (d, i) => (i % 2 === 0 ? '#66ccff' : '#99ff66'))
        .attr('style', 'stroke:rgb(150,150,150);stroke-width:2');
    },

    handleChartLayout() {
      const svg = d3.select('#stacked-svg-tasks');

      this.tasks.forEach((task, taskIndex) => {
        svg
          .selectAll(`.bar${taskIndex}`)
          .data([task])
          .transition()
          .duration(750)
          .attr('transform', `translate(${this.xPosOfTasks[taskIndex]}, 0)`)
          .selectAll('g')
          .attr('transform', (s, i) => this.getPosOfStage(s, i))
          .selectAll('rect')
          .attr('width', this.stageWidth);
      });
    },
  },

  watch: {
    stackedXorY() {
      this.stageWidth = this.getStageWidth();
      this.xPosOfTasks = this.getXPosOfTask(this.GbT, this.tasks, this.stageWidth, this.stackedXorY);
      this.handleChartLayout();
    },
  },
};
</script>
\$\endgroup\$
2
\$\begingroup\$

Overall Feedback

In general, the code looks good and is fairly easy to read. Most of the methods are concise and would lend themselves well to unit tests. I like the use of const by default for variables and only using let for the iterator variable c.

I do have a few recommendations described below that should improve certain aspects of it.

Recommendations

Make id attribute unique

The id attribute “must be unique in the whole document”1, so if you reuse that component, make the value for that attribute unique. It could be a parameter, use a static value (e.g. "id_" + count++), etc.

Avoid the Toll cost of crossing the DOM bridge

bridge toll

”...DOM access is actually pretty costly - I think of it like if I have a bridge - like two pieces of land with a toll bridge, and the JavaScript engine is on one side, and the DOM is on the other, and every time I want to access the DOM from the JavaScript engine, I have to pay that toll”
    - John Hrvatin, Microsoft, MIX09, in this talk Building High Performance Web Applications and Sites at 29:38, also cited in the O'Reilly Javascript book by Nicholas C Zakas Pg 36, as well as mentioned in this post

In your code I see two occurrences of d3.select('#stacked-svg-tasks') and one of d3.selectAll('#stacked-svg-tasks'). The last expression obviously works but the difference between .select() and .selectAll() is that the former uses document.querySelector() whereas the latter uses document.querySelectorAll(), which returns a NodeList instead of a single element - somewhat overkill for getting an element using the id attribute.

In the d3 documentation for .select(selector) we see the following:

If the selector is not a string, instead selects the specified node; this is useful if you already have a reference to a node2

Because you are using VueJS and have ref="stacked-svg-tasks" on the <svg> tag in the template, you have a reference to the target node! You can use $refs instead of querying the DOM each time to select that element.

So lines like this

const svg = d3.select("#stacked-svg-tasks");

Could be updated like this:

const svg = d3.select(this.$refs['stacked-svg-tasks']);

I would recommend switching to camelCase or at least something without the hyphens -

<svg 
  ref="stackedSvgTasks"

Then the dot notation (instead of bracket notation) can be used like below:

const svg = d3.select(this.$refs.stackedSvgTasks);

Additionally, that reference svg could be stored when the component is mounted (e.g. in the mounted method) and re-used in other methods.


Don't Repeat d3 chains over and over again

I know that idiomatic d3.js code typically has chained calls to methods like .selectAll(), .data(), .enter(), etc. And in the chartBuilder() method I see quite a few of those redundant chains. There are at least a few places where those common chains could be abstracted to a method. For instance, if a method was added like:

applyDataToGroup(element, dataMap) {
  return element
    .selectAll('g')
    .data(dataMap)
    .enter()
    .append("g");
}

Then that could be used to greatly simplify the assignment of barChartGroup, taskGroupsand stageGroups:

const barChartGroup = this.applyDataToGroup(this.svg, [this.tasks])
  .attr("class", "bar-chart-group")
  .attr(
    "transform",
    `translate(${this.barChartGroup.x},${this.barChartGroup.y})`
  );

const taskGroups = this.applyDataToGroup(barChartGroup, t => t)
  .attr("class", (t, i) => `bar${i}`)
  .attr("transform", (t, i) => `translate(${this.xPosOfTasks[i]},0)`);

const stageGroupDataMapping = t => this.reconstructedStageData(t.taskStages);
const stageGroups = this.applyDataToGroup(taskGroups, stageGroupDataMapping)
  .attr("transform", (s, i) => this.getPosOfStage(s, i))
  .append("rect")
  .attr("width", this.stageWidth)
  .attr("height", s => this.yScale(s.totalTime))
  .attr("fill", (d, i) => (i % 2 === 0 ? "#66ccff" : "#99ff66"))
  .attr("style", "stroke:rgb(150,150,150);stroke-width:2");

While it would require adding two lines (to declare the method) and a couple extra function calls, it could reduce those 27 lines to the 19 above. To make that method simpler each of those blocks could be abstracted into separate methods.

1https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id
2https://github.com/d3/d3-selection#select

\$\endgroup\$
  • \$\begingroup\$ Sorry for the late response :) I implemented what you have suggested, thanks! \$\endgroup\$ – user4602966 Dec 15 '18 at 18:57
  • \$\begingroup\$ Cool thanks- please consider voting on it. “The first thing you should do after reading someone's answer to your question is vote on the answer, like any user with sufficient reputation does. Vote up answers that are helpful, and vote down answers that give poor advice. Other users will also vote on answers to your question.” -from What should I do when someone answers my question? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 15 '18 at 19:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.