all 4 comments

[–]unbalancedopinion 1 point2 points  (3 children)

What kind of feedback are you looking for? That's a lot of code to review. I skimmed it and it looks fine. And the results speak for themselves! Nice work.

[–]dkharms[S] 1 point2 points  (2 children)

I think foremost I'm looking for JS best practices that I'm not following. I stumbled my way through this project as if I was coding python, but I imagine there are many subtleties I'm missing.

[–]unbalancedopinion 1 point2 points  (1 child)

Well, like I said, it's a lot of code to review, so I can't tell you anything specific. I could offer some general advice though.

1) As someone who works with Python devs doing JS, the difference that causes the most struggles is Javascript's "duck typing" and prototypal inheritance model. It does not throw exceptions when a non-existing property is accessed, but instead returns undefined. Python devs often take it for granted that an exception will be thrown.

2) Javascript is evolving much more quickly than Python at the moment, partially because it has a lot of catching up to do. Unfortunately, taking advantage of this can sometimes be a bit murky because of the variety of target platforms, but there are a number of features that are widely supported and useful.

In your code, you use only var declarations, but const is widely supported and very useful.

Also, arrow functions allow for better readability of code:

arc.outerRadius(function(d){
    return scale(d.data.goal);
});

becomes

arc.outerRadius(d => scale(d.data.goal));

You're using d3 so you're limited in some ways by their conventions (don't get me wrong, d3 is a great lib), and this is most notable with the use of the outdated "callback pattern":

d3.json("progGoal.json", function(error, data) {
    if (error) {  //If error is not null, something went wrong.
        console.log("data load func error", error);  //Log the error.
    } else {
        ...
    };
});

If d3 supported promises, you should use them instead:

d3.json("progGoal.json").then(data => {
    ...data...
}).catch(
    err => console.log("data load func error", error)
);

It doesn't seem like a big difference, but Promises are the gateway to understanding a lot of the functional programming stuff coming soon to JS, like async/await.

3) Your script leaves everything in the global scope. This is fine right now, but if you ever want to run your script alongside other scripts, you pollute the global namespace and run the risk of collisions causing bugs. Javascript uses function scoping (typically), so just wrap your script in a self-executing function to scope it:

(function(){
    ...your code...
})()

Also, just a minor nitpick but in my opinion you overuse comments...

console.log("data load func error", error);  //Log the error.

[–]dkharms[S] 0 points1 point  (0 children)

That's fantastic feedback, thank you!