3
\$\begingroup\$

I'm developing a legend toggling d3.js pie chart application using this jsFiddle as my latest version.

I am aiming to get a streamlined working example where the legend can toggle the slices, trying to deactivate all slices - resorts in a reset which reactivates all the slices. Splitting up presentation and application layer logic.

Tweening needs improvement too - as the slices pop into existence then re-tween smoothly.

How do I improve/fix the various bugs in this code base?

onLegendClick: function(dt, i){
                    //_toggle rectangle in legend

var completeData = jQuery.extend(true, [], methods.currentDataSet);




                    newDataSet = completeData;                        
                    if(methods.manipulatedData){
                        newDataSet = methods.manipulatedData;
                    }


                    d3.selectAll('rect')
                    .data([dt], function(d) { 
                        return d.data.label;
                    })
                    .style("fill-opacity", function(d, j) {
                        var isActive = Math.abs(1-d3.select(this).style("fill-opacity"));
                        if(isActive){
                            newDataSet[j].total = completeData[j].total;                                
                        }else{
                            newDataSet[j].total = 0;                            
                        }

                        return isActive;
                    });                            

                    //animate slices
                    methods.animateSlices(newDataSet);

                    //stash manipulated data
                    methods.manipulatedData = newDataSet;

                }
\$\endgroup\$
2
\$\begingroup\$

CodeReview is more about reviewing code than fixing various bugs, here goes my once over:

  • Use a beautifier, either TidyUp in jsfiddle or jsbeautifier.org, your code is badly indented and is hard to follow because of it
  • Use more meaningful names:
    • onLegendClick: function(dt, i) { <- I am guessing dt is data, i could be anything
    • .style("fill-opacity", function(d, j) <- Same here
  • Commenting is hard, only comment what is not obvious and the general purpose of code blocks

    • This is too obvious:

      //animate slices
      methods.animateSlices(newDataSet);
      
    • This is not, but there is no comment:

      var isActive = Math.abs(1 - d3.select(this).style("fill-opacity"));
      
    • Not sure if this is a bug, or something I dont grasp, why are you referring to completeData even though you might be running of methods.manipulatedData, if this is correct then you should have a comment there

       newDataSet[j].total = completeData[j].total;
      
  • This could use shortcut syntax:

    newDataSet = completeData;
    if(methods.manipulatedData) {
      newDataSet = methods.manipulatedData;
    }
    

    becomes then

    newDataSet = methods.manipulatedData || completeData;
    
  • This could also use a short cut syntax:

    if(isActive) {
      newDataSet[j].total = completeData[j].total;
    } else {
      newDataSet[j].total = 0;
    }
    

    becomes then

    newDataSet[j].total = isActive ? completeData[j].total : 0;
    
  • Do not pollute the global namespace, newDataSet is used without a var declaration

All in all, I think you could have pasted more code to review, we tend to not review the code in the jsfiddle, only what you pasted. My counter proposal would look like this:

onLegendClick: function(dt, i) {

  var completeData = jQuery.extend(true, [], methods.currentDataSet),
      newDataSet = methods.manipulatedData || completeData;

  d3.selectAll('rect')
    .data([dt], function(d) {
      return d.data.label;
    })
    .style("fill-opacity", function(d, j) {
      //Witty comment on how this works
      var isActive = Math.abs(1 - d3.select(this).style("fill-opacity"));
      //Witty comment as to why I still need completeData
      newDataSet[j].total = isActive ? completeData[j].total : 0;
      return isActive;
    });

  methods.animateSlices(newDataSet);
  methods.manipulatedData = newDataSet;
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the review - well completeData is the data intact. Its included - in case this is the first time a toggle has occurred - so before manipulatedData exists. jsfiddle.net/Qh9X5/3333 \$\endgroup\$ – Rob Oct 27 '14 at 14:45
  • \$\begingroup\$ Tried to make the labels more sensical - jsfiddle.net/Qh9X5/3340 \$\endgroup\$ – Rob Oct 27 '14 at 16:50
0
\$\begingroup\$

Here is the entire js code - I've used the tidyup. I wasn't sure about using the shortcuts as I'm not sure the values will be correct.

$(document).ready(function () {

    var pieChart = {
        el: "",
        init: function (el, options) {
            var clone = jQuery.extend(true, {}, options["data"]);

            pieChart.el = el;
            pieChart.setup(clone, options["width"], options["height"], options["r"], options["ir"]);
        },
        getArc: function (radius, innerradius) {
            var arc = d3.svg.arc()
                .innerRadius(innerradius)
                .outerRadius(radius);

            return arc;
        },
        setup: function (dataset, w, h, r, ir) {

            var padding = 80;

            this.width = w;
            this.height = h;
            this.radius = r
            this.innerradius = ir;

            this.color = d3.scale.category20();

            this.pie = d3.layout.pie()
                .sort(null)
                .value(function (d) {
                return d.total;
            });

            this.arc = this.getArc(this.radius, this.innerradius);

            this.svg = d3.select(pieChart.el["selector"]).append("svg")
                .attr("width", this.width + padding)
                .attr("height", this.height + padding);

            this.holder = this.svg.append("g")
                .attr("transform", "translate(" + ((this.width / 2) + (padding / 2)) + "," + ((this.height / 2) + (padding / 2)) + ")");

            this.piec = this.holder.append("g")
                .attr("class", "piechart");

            this.segments = this.holder.append("g")
                .attr("class", "segments");

            this.labels = this.holder.append("g")
                .attr("class", "labels");

            this.pointers = this.holder.append("g")
                .attr("class", "pointers");

            this.legend = this.svg.append("g")
                .attr("class", "legend")
                .attr("transform", "translate(" + -(this.width / 4) + "," + this.height + ")");

        },
        oldPieData: "",
        pieTween: function (r, ir, d, i) {
            var that = this;

            var theOldDataInPie = pieChart.oldPieData;
            // Interpolate the arcs in data space

            var s0;
            var e0;

            if (theOldDataInPie[i]) {
                s0 = theOldDataInPie[i].startAngle;
                e0 = theOldDataInPie[i].endAngle;
            } else if (!(theOldDataInPie[i]) && theOldDataInPie[i - 1]) {
                s0 = theOldDataInPie[i - 1].endAngle;
                e0 = theOldDataInPie[i - 1].endAngle;
            } else if (!(theOldDataInPie[i - 1]) && theOldDataInPie.length > 0) {
                s0 = theOldDataInPie[theOldDataInPie.length - 1].endAngle;
                e0 = theOldDataInPie[theOldDataInPie.length - 1].endAngle;
            } else {
                s0 = 0;
                e0 = 0;
            }

            var i = d3.interpolate({
                startAngle: s0,
                endAngle: e0
            }, {
                startAngle: d.startAngle,
                endAngle: d.endAngle
            });

            return function (t) {
                var b = i(t);
                return pieChart.getArc(r, ir)(b);
            };
        },
        removePieTween: function (r, ir, d, i) {
            var that = this;
            s0 = 2 * Math.PI;
            e0 = 2 * Math.PI;
            var i = d3.interpolate({
                startAngle: d.startAngle,
                endAngle: d.endAngle
            }, {
                startAngle: s0,
                endAngle: e0
            });

            return function (t) {
                var b = i(t);
                return pieChart.getArc(r, ir)(b);
            };
        },
        animateSlices: function (dataSet) {

            var r = $(pieChart.el["selector"]).data("r");
            var ir = $(pieChart.el["selector"]).data("ir");

            this.piedata = pieChart.pie(dataSet);

            //__slices
            this.path = pieChart.segments.selectAll("path.pie")
                .data(this.piedata, function (d) {
                return d.data.label
            });

            this.path.enter().append("path")
                .attr("class", "pie")
                .attr("fill", function (d, i) {
                return pieChart.color(i);
            })
                .attr("stroke", "#ffffff")
                .transition()
                .duration(300)
                .attrTween("d", function (d, i) {
                return pieChart.pieTween(r, ir, d, i);
            });

            this.path.transition()
                .duration(300)
                .attrTween("d", function (d, i) {
                return pieChart.pieTween(r, ir, d, i);
            });

            this.path.exit()
                .transition()
                .duration(300)
                .attrTween("d", function (d, i) {
                return pieChart.removePieTween(r, ir, d, i);
            })
                .remove();
            //__slices

            //__labels  
            var labels = pieChart.labels.selectAll("text")
                .data(this.piedata, function (d) {
                return d.data.label
            });

            labels.enter()
                .append("text")
                .attr("text-anchor", "middle")

            labels.attr("x", function (d) {
                var a = d.startAngle + (d.endAngle - d.startAngle) / 2 - Math.PI / 2;
                d.cx = Math.cos(a) * (ir + ((r - ir) / 2));
                return d.x = Math.cos(a) * (r + 20);
            })
                .attr("y", function (d) {
                var a = d.startAngle + (d.endAngle - d.startAngle) / 2 - Math.PI / 2;
                d.cy = Math.sin(a) * (ir + ((r - ir) / 2));
                return d.y = Math.sin(a) * (r + 20);
            })
                .attr("opacity", function (d) {
                var opacityLevel = 1;
                if (d.value == 0) {
                    opacityLevel = 0;
                }
                return opacityLevel;
            })
                .text(function (d) {
                return d.data.label;
            })
                .each(function (d) {
                var bbox = this.getBBox();
                d.sx = d.x - bbox.width / 2 - 2;
                d.ox = d.x + bbox.width / 2 + 2;
                d.sy = d.oy = d.y + 5;
            })
                .transition()
                .duration(300)

            labels.transition()
                .duration(300)

            labels.exit().remove();
            //__labels

            //__pointers
            pieChart.pointers.append("defs").append("marker")
                .attr("id", "circ")
                .attr("markerWidth", 6)
                .attr("markerHeight", 6)
                .attr("refX", 3)
                .attr("refY", 3)
                .append("circle")
                .attr("cx", 3)
                .attr("cy", 3)
                .attr("r", 3);

            var pointers = pieChart.pointers.selectAll("path.pointer")
                .data(this.piedata, function (d) {
                return d.data.label
            });

            pointers.enter()
                .append("path")
                .attr("class", "pointer")
                .style("fill", "none")
                .style("stroke", "black")
                .attr("marker-end", "url(#circ)");

            pointers.attr("d", function (d) {
                if (d.cx > d.ox) {
                    return "M" + d.sx + "," + d.sy + "L" + d.ox + "," + d.oy + " " + d.cx + "," + d.cy;
                } else {
                    return "M" + d.ox + "," + d.oy + "L" + d.sx + "," + d.sy + " " + d.cx + "," + d.cy;
                }
            })
                .attr("opacity", function (d) {
                var opacityLevel = 1;
                if (d.value == 0) {
                    opacityLevel = 0;
                }
                return opacityLevel;
            })
                .transition()
                .duration(300)

            pointers.transition()
                .duration(300)

            pointers.exit().remove();
        },
        onToggle: function (sliceData, index) {
            //_toggle rectangle in legend

            //_toggle slice
            var completeData = jQuery.extend(true, [], pieChart.currentDataSet);
            var dataLength = completeData.length;

            var newDataSet = completeData;
            if (pieChart.manipulatedData) {
                newDataSet = pieChart.manipulatedData;
            }

            d3.selectAll('rect')
                .data([sliceData], function (d) {
                return d.data.label;
            })
                .style("fill-opacity", function (d) {
                var isActive = Math.abs(1 - d3.select(this).style("fill-opacity"));
                if (isActive) {
                    newDataSet[index].total = completeData[index].total;
                    newDataSet[index].value = completeData[index].value;
                } else {
                    newDataSet[index].total = 0;
                    newDataSet[index].value = 0;
                }

                return isActive;
            });

            //if all elements are to be not shown - reset to show all slices again.
            //animate slices
            pieChart.animateSlices(newDataSet);

            //stash manipulated data
            pieChart.manipulatedData = newDataSet;
        },
        update: function (el, dataSet) {
            var that = this;

            pieChart.el = el;

            pieChart.svg = d3.select(pieChart.el["selector"] + " .piechart");

            pieChart.segments = d3.select(pieChart.el["selector"] + " .segments");
            pieChart.labels = d3.select(pieChart.el["selector"] + " .labels");
            pieChart.pointers = d3.select(pieChart.el["selector"] + " .pointers");

            pieChart.legend = d3.select(pieChart.el["selector"] + " .legend");

            dataSet.forEach(function (d) {
                d.total = +d.value;
            });

            pieChart.currentDataSet = dataSet;
            pieChart.animateSlices(dataSet);

            //__legends
            var w = 200;

            // add legend   
            var legend = pieChart.legend; //.append("g")

            var legendRects = legend.selectAll('rect')
                .data(this.piedata, function (d) {
                return d.data.label
            });

            legendRects.enter()
                .append("rect")
                .attr("x", w - 65)
                .attr("y", function (d, i) {
                return i * 20;
            })
                .attr("width", 10)
                .attr("height", 10)
                .style("fill", function (d, i) {
                return pieChart.color(i);
            })
                .style("stroke", function (d, i) {
                return pieChart.color(i);
            })
            .on('click', function(d, i){
                pieChart.onToggle(d, i);
            })
                .transition()
                .duration(300)

            legendRects.style("fill", function (d, i) {
                return pieChart.color(i);
            })
                .style("stroke", function (d, i) {
                return pieChart.color(i);
            })
                .transition()
                .duration(300)

            legendRects.exit().remove();

            var legendText = legend.selectAll('text.label')
                .data(this.piedata, function (d) {
                return d.data.label
            });

            legendText.enter()
                .append("text")
                .attr("class", "label")
                .attr("x", w - 52)
                .attr("y", function (d, i) {
                return i * 20 + 9;
            })
                .text(function (d) {
                return d.data.label;
            })
                .transition()
                .duration(300)

            legendText.text(function (d) {
                return d.data.label;
            })
                .transition()
                .duration(300)

            legendText.exit().remove();

            var legendTextVals = legend.selectAll('text.vals')
                .data(this.piedata, function (d) {
                return d.data.label
            });

            legendTextVals.enter()
                .append("text")
                .attr("class", "vals")
                .attr("x", w + 20)
                .attr("y", function (d, i) {
                return i * 20 + 9;
            })
                .text(function (d) {
                return d.data.value;
            })
                .transition()
                .duration(300)

            legendTextVals.text(function (d) {
                return d.data.value;
            })
                .transition()
                .duration(300)

            legendTextVals.exit().remove();

            //__pointers

            this.oldPieData = this.piedata;
        }
    };

    var dataCharts = [{
        "data": [{
            "segments": [{
                "label": "apple",
                    "value": 53245
            }, {
                "label": "cherry",
                    "value": 145
            }, {
                "label": "pear",
                    "value": 2245
            }, {
                "label": "bananana",
                    "value": 15325
            }]
        }]
    }, {
        "data": [{
            "segments": [{
                "label": "milk",
                    "value": 122
            }, {
                "label": "cheese",
                    "value": 44
            }, {
                "label": "grapes",
                    "value": 533
            }]
        }]
    }, {
        "data": [{
            "segments": [{
                "label": "pineapple",
                    "value": 1532
            }, {
                "label": "orange",
                    "value": 1435
            }, {
                "label": "grapes",
                    "value": 22
            }]
        }]
    }, {
        "data": [{
            "segments": [{
                "label": "lemons",
                    "value": 133
            }, {
                "label": "mango",
                    "value": 435
            }, {
                "label": "melon",
                    "value": 2122
            }]
        }]
    }];

    var clone = jQuery.extend(true, {}, dataCharts);

    //__invoke concentric
    $('[data-role="piechart"]').each(function (index) {
        var selector = "piechart" + index;

        $(this).attr("id", selector);

        var options = {
            data: clone[0].data,
            width: $(this).data("width"),
            height: $(this).data("height"),
            r: $(this).data("r"),
            ir: $(this).data("ir")
        }

        pieChart.init($("#" + selector), options);
        pieChart.update($("#" + selector), clone[0].data[0].segments);
    });

    $(".testers a").on("click", function (e) {
        e.preventDefault();

        var clone = jQuery.extend(true, {}, dataCharts);

        var min = 0;
        var max = 3;

        //__invoke pie chart
        $('[data-role="piechart"]').each(function (index) {
            pos = Math.floor(Math.random() * (max - min + 1)) + min;
            pieChart.update($("#" + $(this).attr("id")), clone[pos].data[0].segments);
        });
    });
});
\$\endgroup\$

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.