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

Minimize use of global variables #244

Open
ghing opened this issue Oct 3, 2017 · 3 comments
Open

Minimize use of global variables #244

ghing opened this issue Oct 3, 2017 · 3 comments

Comments

@ghing
Copy link
Contributor

ghing commented Oct 3, 2017

I noticed that there are a lot of global variables used in the State grid map template.

Use of global variables can make it difficult to reason about control flow in programs and make testing code difficult.

They also make it tempting to use global variables in customizations to the template, even though it's not terribly difficult to pass shared values as function arguments.

At the very least, globals should be limited to the items that come from the upstream spreadsheets such as DATA and LABELS, though this is probably unnecessary as well. Or all global state could be consolidated into a single shared global object.

@ghing ghing changed the title Minimize use global variables Minimize use of global variables Oct 3, 2017
@ghing
Copy link
Contributor Author

ghing commented Oct 4, 2017

Here's another example from the bar chart template:

/*
 * Format data for D3.
 */
var formatData = function() {
    DATA.forEach(function(d) {
        d['amt'] = +d['amt'];
    });
}

@katiepark
Copy link
Contributor

This is only slightly relevant, but I think it would be useful to refactor formatData not to operate directly on any global variables but instead accept the input as function parameter and return the output data. This will make it easier to modify templates to draw multiple charts in the same graphic. Here's an example: https://github.com/nprapps/graphics-archive/blob/13446bc55cce13ab29b04ca3496764f5c683a45a/2016/09/north-korea-coal-20160914/js/graphic.js#L34

@ghing
Copy link
Contributor Author

ghing commented Oct 4, 2017

@katiepark haha:

/*
 * Format data for D3.
 *
 * I updated the version from the default version to return a transformed
 * data array instead of modifying `DATA` in place.
 */
var formatData = function(data) {
  return data.map(function(d) {
    return _.extend({}, d, {
        // Convert numers as strings to real numbers.  This method is kind of
        // neat because it works for either integers or floats and doesn't
        // complain if the value is already numeric.
        'amt': +d['amt']
    });
  });
};

I was literally writing that as your comment came in.

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