4
\$\begingroup\$

I am trying to make my script maintenance-free by replacing some "static" variables and labels with dynamic ones. The main function of my code is to extract data from a SharePoint-list which contains a number of list items with several columns (including a column for school), then count the number of items based on a set criteria (e.g. how many from the school "BI") and then use the data to create a pie chart.

Currently, I have manually made up variables, if statements, and highcharts data labels. However, the list of schools is stored in another SharePoint-list which I can extract in JSON format like this: ["(Ikke oppgitt)", "BI", "CBS", "HiOA", "HiST", "NHH", "NMBU", "NTNU", "UiA", "UiO", "Øvrige"] . The "school"-column in the main SharePoint-list is simply a lookup-column from the school-list.

I am trying to make this smarter by first extracting the schools from the SP-list, then create a variable for each school with the value = 0, then make an if statement for each school that counts the number of occurrences of the school, and then push the school name and the corresponding value from the variable to the highchart dataset to create a pie chart. Is there any way to do this? I am trying to do this so that the code does not need to be changed in case a new school is added to the school SP-list.

$.when(queryData).done(function() {
                antallAnsatteBI = 0
                antallAnsatteCBS = 0
                antallAnsatteHiOA = 0
                antallAnsatteHiST = 0
                antallAnsatteNHH = 0
                antallAnsatteNMBU = 0
                antallAnsatteNTNU = 0
                antallAnsatteUiA = 0
                antallAnsatteUiO = 0
                antallAnsatteØvrigeSkoler = 0

                dataqueryData = this.data;
                var datalength = dataqueryData.length;
                for (var i = 0; i < datalength; i++) {
                    var master = dataqueryData[i].Master.lookupValue;
                    if (master !== '(Ikke oppgitt)') {
                        var skole = master
                    } else if (master == '(Ikke oppgitt)') {
                        var bachelor = dataqueryData[i].Bachelor.lookupValue;
                        var skole = bachelor
                    }
                    // count functions
                            if (skole == 'BI') {
                                antallAnsatteBI++
                            } else if (skole == 'CBS') {
                                antallAnsatteCBS++
                            } else if (skole == 'HiOA') {
                                antallAnsatteHiOA++
                            } else if (skole == 'HiST') {
                                antallAnsatteHiST++
                            } else if (skole == 'NHH') {
                                antallAnsatteNHH++
                            } else if (skole == 'NMBU') {
                                antallAnsatteNMBU++
                            } else if (skole == 'NTNU') {
                                antallAnsatteNTNU++
                            } else if (skole == 'UiA') {
                                antallAnsatteUiA++
                            } else if (skole == 'UiO') {
                                antallAnsatteUiO++
                            } else if (skole == 'Øvrige') {
                                antallAnsatteØvrigeSkoler++
                            }
                           
                }
                createSkolerPie();
            });
        }

        function createSkolerPie() {

            $(function() {
                $('#container_skoler').highcharts({
                    series: [{
                        type: 'pie',
                        name: 'Skole',
                        data: [
                            ['BI', antallAnsatteBI],
                            ['CBS', antallAnsatteCBS],
                            ['HiOA', antallAnsatteHiOA],
                            ['HiST', antallAnsatteHiST],
                            ['NHH', antallAnsatteNHH],
                            ['NMBU', antallAnsatteNMBU],
                            ['NTNU', antallAnsatteNTNU],
                            ['UiA', antallAnsatteUiA],
                            ['UiO', antallAnsatteUiO],
                            ['Øvrige', antallAnsatteØvrigeSkoler]
                        ]
                    }]
                });
            });
        }

\$\endgroup\$
3
\$\begingroup\$

First, and I say this as a fellow Scandinavian: Just stick to English in your code. It's just easier. As long as you've got words like while, if, throw etc., mixing languages is just strange.

It's even stranger when you're mixing languages within the same word, like createSkolerPie. (Funnily enough, your one comment is in English, while the code is mixed.)

Sure, you can code in any language if you want, but keeping things in English helps readability. You wouldn't unnecessarily mix languages in an essay or letter, would you? So why mix languages in code?

Besides, you may not be the only person to ever work on the code (for instance, here I am, reviewing it! I just happen to understand it, but that's pure chance), and English is the lingua franca among programmers.

As for the code itself:

You're creating a lot of global variables. All your antallAnsatte* ("numberOfEmployees*", I believe) variables are global, because you didn't properly declare them with the var keyword. This is really bad practice.

I know you want to use the variables in createSkolerPie, but just pass them to the function then. It's what function arguments are for. And it doesn't explain why dataqueryData is global; that one isn't used elsewhere.

dataqueryData is a bit of an odd duck for another reason too: There's no need for it. You're not giving the value a more descriptive name, and it's in fact shorter to write this.data.

You're also missing a few semi-colons, and you've got an extra } on line 50.

Also, if you ever need to parse a bunch of mutually exclusive conditions, don't use if.. else if.. else if..: Use a switch statement.

Anyway, you say you get a list of schools, and then... you write that same list again as a bunch of variables? That can't be right. You say you want it to be more maintainable, yet you hardcode every single school name into the code. And repeat them over and over. This is not very maintainable. It's error prone, and it's just needlessly laborious and repetitive.

Now, I don't see that school list anywhere in your code, but you gave an example of its contents in your question, so I'll just use that in the code below.

Anyway, first order of business: Create an object (aka a hash, aka a dictionary, aka an associative array, aka a map) using the school names as keys, and set zero for the values:

// I don't know how you get this from the server, so just pasted it in here.
// In fact, you could just hard-code it like this and still make the rest
// of the code simpler
var schools = ["(Ikke oppgitt)", "BI", "CBS", "HiOA", "HiST", "NHH", "NMBU", "NTNU", "UiA", "UiO", "Øvrige"];

var employeeCounts = {}; // empty object

// create key/value pairs for each school
for(var i = 0, l = schools.length ; i < l ; i++) {
  employeeCounts[schools[i]] = 0;
}

Or, if you want to be fancy and use reduce you can do:

var employeeCounts = schools.reduce(function (memo, name) {
  memo[name] = 0;
  return memo;
}, {});

In either case, you end up with an object with all the school names as keys, and zeros for the counts.

Now, when you parse the other data, you can do this:

var NOT_GIVEN = '(Ikke oppgitt)'; // stick this in a "constant" to avoid repeating it

for(var i = 0, l = this.data.length ; i < l ; i++) {
  var schoolName = this.data[i].Master.lookupValue;

  if(schoolName === NOT_GIVEN) {
    schoolName = this.data[i].Bachelor.lookupValue;
  }

  // you could write the above as a ternary, but it's not my style.
  // But you want to, it could look like this (or be on one long line):
  //
  // var schoolName = this.data[i].Master.lookupValue !== NOT_GIVEN
  //                  ? this.data[i].Master.lookupValue
  //                  : this.data[i].Bachelor.lookupValue;


  if(schoolName in employeeCounts) { // an extra check, for safety
    employeeCounts[schoolName]++;
  }
}

That's it - that's all the counting done.

Now just convert it to an array highcharts can use:

var chartData = [];
for(var i = 0, l = schools.length ; i < l ; i++) { // using the schools array again to preserve the sorting
  if(schools[i] !== NOT_GIVEN) { // filter out the
    charData.push([schools[i], employeeCounts[schools[i]]]);
  }
}

Or, again, you can use reduce:

var chartData = schools.reduce(function (memo, name) {
  if(name !== NOT_GIVEN) {
    memo.push([name, employeeCounts[name]]);
  }
  return memo;
}, []);

Putting it all together you'd get something like:

$.when(queryData).done(function () {
  var NOT_GIVEN = '(Ikke oppgitt)';

  // again, this shouldn't be hardcoded here - grab it from the server instead
  var schools = ["(Ikke oppgitt)", "BI", "CBS", "HiOA", "HiST", "NHH", "NMBU", "NTNU", "UiA", "UiO", "Øvrige"];

  var employeeCounts = schools.reduce(function (memo, name) {
    memo[name] = 0;
    return memo;
  }, {});

  // let's use forEach here instead, just because
  this.data.forEach(function (datum) {
    var schoolName = datum.Master.lookupValue;
    if(schoolName === NOT_GIVEN) {
      schoolName = datum.Bachelor.lookupValue;
    }
    if(schoolName in employeeCounts) { // an extra check, for safety
      employeeCounts[schoolName]++;
    }
  });

  var chartData = schools.reduce(function (memo, name) {
    if(name !== NOT_GIVEN) {
      memo.push([name, employeeCounts[name]]);
    }
    return memo;
  }, []);

  // skipping the separate function here, since the function was hard-coupled
  // to this code anyway
  $('#container_skoler').highcharts({
    series: [{
      type: 'pie',
      name: 'Skole',
      data: chartData
    }]
  });
});

And done.

Alternatively, you could skip loading the list of schools, and just group the data:

$.when(queryData).done(function () {
  var NOT_GIVEN = '(Ikke oppgitt)';

  // group the data
  var employeeCounts = this.data.reduce(function (memo, datum) {
    var schoolName = datum.Master.lookupValue;
    if(schoolName === NOT_GIVEN) {
      schoolName = datum.Bachelor.lookupValue;
    }
    memo[schoolName] = memo[schoolName] || 0; // create the key if it doesn't exist
    memo[schoolName]++;
  }, {});

  var chartData = Object.keys(employeeCounts).reduce(function (memo, name) {
    if(name !== NOT_GIVEN) {
      memo.push([name, employeeCounts[name]]);
    }
    return memo;
  }, []);

  // you may want to sort the counts, since they'll otherwise be pretty much random
  // This one will sort the data by count in descending order
  chartData.sort(function (a, b) { return b[1] - a[1]; });

  // skipping the separate function here, since the function was hard-coupled
  // to this code anyway
  $('#container_skoler').highcharts({
    series: [{
      type: 'pie',
      name: 'Skole',
      data: chartData
    }]
  });
});

This won't include schools where there sum was zero, but that may be a good thing (zero doesn't show up on a pie chart anyway).

You'll note I didn't wrap the chart-creation as an on-document-ready callback (i.e. wrapped in $(...)). It's better to do all the data fetching when the document is ready, and thus, when that's done, the document is already good to go.

Also, presuming you're using jQuery to do the fetching, you don't need $.when(queryData).done(...); you can just chain off of queryData itself:

queryData.done(function () { ... });

Finally, if you're going to be doing a lot of this stuff, consider using underscore.js or lo-dash. Makes it easier to handle data sets (filtering, grouping, counting...)

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for your help, Flambino! I was able to accomplish what I was looking for, because of your answer. I am now going through my code to clean up and make it easier to maintain. \$\endgroup\$ – Daniel G. R. Nov 8 '14 at 16:13
  • \$\begingroup\$ @DanielG.R. You're welcome. Click the checkmark by my answer to mark your question as answered (or you can wait, but this question's been here a while, so I don't think any new answers will show up) \$\endgroup\$ – Flambino Nov 8 '14 at 16:19

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.