2
\$\begingroup\$

At first I calculate the total of each data then give each data of data a percentage base on total. The percentage is calculated by adding its percentage and sum of all percentage before it. I then calculate a new value by multiplying percentage and total. I finally sort them from largest to smallest.

How can I reduce the number of .each loops?

var data= [
    {
        "data": [
            {
                "value": "5700",
                "color": "#1ca2d5",
                "label": "Shortlisted"
            },
            {
                "value": "2570",
                "color": "#ed5d67",
                "label": "Interviewed"
            },
            {
                "value": "1054",
                "color": "#66BB6A",
                "label": "Finallisted"
            }
        ],
        "under": "Top"
    },
{
    "data": [
        {
            "value": "2847",
            "color": "#1ca2d5",
            "label": "Shortlisted"
        },
        {
            "value": "3541",
            "color": "#ed5d67",
            "label": "Interviewed"
        },
        {
            "value": "1486",
            "color": "#66BB6A",
            "label": "Finallisted"
        }
    ],
    "under": "Mid"
},
{
    "data": [
        {
            "value": "16900",
            "color": "#1ca2d5",
            "label": "Shortlisted"
        },
        {
            "value": "12900",
            "color": "#ed5d67",
            "label": "Interviewed"
        },
        {
            "value": "5780",
            "color": "#66BB6A",
            "label": "Finallisted"
        }
    ],
    "under": "Dhsanullah University of Science and Technology"
},
{
    "data": [
        {
            "value": "5700",
            "color": "#1ca2d5",
            "label": "Shortlisted"
        },
        {
            "value": "2570",
            "color": "#ed5d67",
            "label": "Interviewed"
        },
        {
            "value": "1054",
            "color": "#66BB6A",
            "label": "Finallisted"
        }
    ],
    "under": "TTop"
},
    {
        "data": [
           {
               "value": "5700",
               "color": "#1ca2d5",
               "label": "Shortlisted"
           },
           {
               "value": "2570",
               "color": "#ed5d67",
               "label": "Interviewed"
           },
           {
               "value": "1054",
               "color": "#66BB6A",
               "label": "Finallisted"
           }
        ],
        "under": "Tcop"
    }
];

var tempData=[];
$.each(data, function (i, d) {
            var total = 0;
            $.each(d.data, function (j, obj) {
                total += parseInt(obj.value);
            });
            d["total"] = total.toString();
        });

        $.each(data, function (i, d) {
            var percentage = 0;
            $.each(d.data, function (j, obj) {
                obj["original"] = obj.value;
                var per = (parseInt(obj.value) * 100) / parseInt(d.total);
                percentage += per;
                obj["percentage"] = per;
                obj.value = (parseInt(d.total) * percentage) / 100;
                obj["under"] = d.under;
            });

        });
        $.each(data, function (i, d) {
            d.data.sort(function (a, b) { return b.value - a.value; });
            $.each(d.data, function (j, obj) {
                tempData.push(obj);
            });
        });
\$\endgroup\$
  • \$\begingroup\$ FYI: I asked this question on SO based on this question. It might contain some fun information. \$\endgroup\$ – Sumurai8 Jan 2 '16 at 19:03
6
\$\begingroup\$

Yes, you can reduce the number of $.each(), and also simplify the whole code, in several ways.

Note that to improve readability I replaced some variable names:

. data replaced by source (to avoid confusing with data members in objects)

. d replaced by objSet inside of functions (more significant, as it is really a set of obj)

. all i and j replaced by index (more significant, and don't need to be different inside of different functions)

. tempData --> result

That's said, the first simplification come from the fact you don't need to iterate three times on the (now said) source, because there is no interaction between different objSets of source.
So instead of:

var tempData=[];
$.each(data, function (i, d) {
  // first step
});

$.each(data, function (i, d) {
  // second step
});

$.each(data, function (i, d) {
  // third step
});

it becomes:

var result=[];
$.each(source, function (index, objSet) {
  // first step
  // second step
  // third step
});

Then we can simplify each step...

Computing total can be achieved without $.each so this step:

var total = 0;
$.each(d.data, function (j, obj) {
    total += parseInt(obj.value);
});
d["total"] = total.toString();

can be achieved using reduce():

  objSet.total = objSet.data.reduce(function(total, obj) {
      return total + parseInt(obj.value);
  }, 0);

or even simpler in ES6:

objSet.total = objSet.data.reduce((total, obj) => total + parseInt(obj.value), 0);

The second step can use map() instead of $.each(), and also be slightly alleviated avoiding the temporary per variable. So rather than:

var percentage = 0;
$.each(d.data, function (j, obj) {
    obj["original"] = obj.value;
    var per = (parseInt(obj.value) * 100) / parseInt(d.total);
    percentage += per;
    obj["percentage"] = per;
    obj.value = (parseInt(d.total) * percentage) / 100;
    obj["under"] = d.under;
});

it can be:

  var percentage = 0;
  objSet.data = objSet.data.map(function (obj) {
    obj.original = obj.value;
    obj.percentage = (parseInt(obj.value) * 100) / parseInt(objSet.total);
    percentage += obj.percentage;
    obj.value = (parseInt(objSet.total) * percentage) / 100;
    obj.under = objSet.under;
    return obj;
  });

EDIT after realizing it could be yet more "compressed" using reduce() instead of map():

  objSet.data.reduce(function (percentage, obj) {
    obj.original = obj.value;
    obj.percentage = (parseInt(obj.value) * 100) / parseInt(objSet.total);
    percentage += obj.percentage;
    obj.value = (parseInt(objSet.total) * percentage) / 100;
    obj.under = objSet.under;
    return return percentage;
  }, 0);

It can be seen as a strange way using reduce(): it's usually employed to give a final result, while here it only avoids declaring a temporary variable for percentage.
Also strange, at first view, is that we keep getting a modified version of objSet.data without map(), because it is directly set.

Finally the third step can take advantage of concat to again avoid using $.each(), so this:

d.data.sort(function (a, b) { return b.value - a.value; });
$.each(d.data, function (j, obj) {
    tempData.push(obj);
});

becomes:

  objSet.data.sort(function (a, b) { return b.value - a.value; });
  result = result.concat(...objSet.data);

To summarize, the whole code can be reduced to this:

var result=[];
$.each(source, function (index, objSet) {

  objSet.total = objSet.data.reduce((total, obj) => total + parseInt(obj.value), 0);

  objSet.data.reduce(function (percentage, obj) {
    obj.original = obj.value;
    obj.percentage = (parseInt(obj.value) * 100) / parseInt(objSet.total);
    percentage += obj.percentage;
    obj.value = (parseInt(objSet.total) * percentage) / 100;
    obj.under = objSet.under;
    return return percentage;
  }, 0);

  objSet.data.sort(function (a, b) { return b.value - a.value; });
  result = result.concat(...objSet.data);
});
\$\endgroup\$
2
\$\begingroup\$

I think you need the three inner loops but can get away with just two outer loops by moving the third inner loop into the second outer.

In addition, I would choose to do as follows :

  • In the first inner loop, calculate the totals is cleaner with .reduce() rather than .each().
  • In the second inner loop, avoid the need for * 100 and later / 100 by working with proportions instead of percentages.
  • Move the obj.original and obj.under assignments from the second inner loop to the first inner loop, leaving the second inner loop to address just the proportions.
  • Avoid parseInt() by relying on javascript's automatic typing, and using + for coercion to Number.
  • Work with dot syntax foo.property, not associative syntax foo["property"].

Also, I think the sort can be avoided. It seems that after the second loop, the values within each of the "data" arrays are cumulative, therefore already sorted ascending. If I'm right then all you need to do in the third loop for a descending sort order is d.data.reverse();

var tempData = [];
$.each(data, function (i, d) {
    d.total = d.data.reduce(function (t, obj) {
        obj.original = obj.value;
        obj.under = d.under;
        return t + +obj.value;
    }, 0);
});
$.each(data, function (i, d) {
    var proportion = 0;
    $.each(d.data, function (j, obj) {
        var prop = obj.value / d.total;
        proportion += prop;
        obj.percentage = 100 * prop;
        obj.value = d.total * proportion;
    });
    d.data.reverse();
    $.each(d.data, function (j, obj) {
        tempData.push(obj);
    });
});

You could probably contrive a way to handle the reversal and push objects onto tempData in the second inner loop but at the cost of readability.

Overall, the above changes are mainly cosmetic. You almost certainly won't notice any performance improvements unless the data arrays are huge. For small data arrays, I would be quite happy with the original code - maybe just do the .reverse() (if it's correct) instead of .sort().


Edit:

After reading the answers by @cFreed and @Sumurai8, I can see that my answer can be improved quite a bit - a solution of just 9 lines. I don't want to overtly steal from the other guys but will post my revised answer if asked by the OP.


OK, with just 15 minutes of 2015 left to go (GMT) and with some shameless cribbing of ideas from @cFreed's and @Sumurai8's answers ...

  • use of .map() - @cFreed
  • use of .concat() - - @cFreed & @Sumurai8
  • single outer jQuery.each() loop - @cFreed & @Sumurai8
  • avoid unnecessary computing - @Sumurai8
  • non-destuction of the original data - @Sumurai8

... plus my own injection of jQuery.extend()`, I ended up with this :

var tempData = [];
$.each($.extend(true, {}, data), function(i, d) {
    var total = d.data.reduce(function(t, obj) {
        return (obj.cumulative = t + +obj.value);
    }, 0);
    tempData = tempData.concat(d.data.map(function(obj) {
        return $.extend(obj, { 'percentage': 100 * obj.cumulative / total, 'under': d.under });
    }).reverse());
});

Notes:

  • The overall pattern is effectively one outer .each() loop with inner .reduce() and .map() loops.
  • First jQuery.extend() clones data
  • Second jQuery.extend() decorates obj
  • d.data.reduce(...) sums d.data's values.
  • The pattern tempData.concat(d.data.map((obj)=>...)) first provides each obj with some additional properties, then concatenates the resulting mapped array of decorated objects to tempData.
\$\endgroup\$
  • \$\begingroup\$ Like you, I see multiple possible improvements after reading your answer and the @Summurai8's one. So, if he agrees, I propose that you post the answer that you think, and each of us might refine it if he finds it possible. \$\endgroup\$ – cFreed Dec 31 '15 at 23:05
  • \$\begingroup\$ Sure, go ahead. \$\endgroup\$ – Sumurai8 Dec 31 '15 at 23:09
  • \$\begingroup\$ This is what I like abut Code Review, compared with StackOverflow - more cooperation, less competition. \$\endgroup\$ – Roamer-1888 Jan 1 '16 at 1:09
  • \$\begingroup\$ Wow, very impressive! Notably for its compacity. But I must admit that I can't fully understand it without a deeper examination, while I'm just to get away for a week. So I'll give back my observations after that. \$\endgroup\$ – cFreed Jan 1 '16 at 12:39
  • \$\begingroup\$ I would still suggest using better variable names, but that function looks great. It has me wondering if there is some construction where one can defer the calculation of percentage until after the total has been calculated, without doing, essentially, a second loop through the data. \$\endgroup\$ – Sumurai8 Jan 1 '16 at 19:31
1
\$\begingroup\$

Styling and readability

You are using inconsistent indentation. This makes the code much harder to read, and will cause developers to incorrectly assume code belongs to a different block than it actually does. I recommend fixing this.

You are using non-descriptive variable names. Using i and j is probably fine. Those are commonly used as indices. d is supposedly a shorthand for data, which is also the object it was in, and it has a property data as well.

You are using questionable variables names, such as tempData. I would expect it to contain temporary data, but it seems to be the output of your code?

You are polluting the global scope with variables. Consider putting the code in an IIFE, and putting this particular code in it's own function. This will also make it clear what the output of that function actually is.

You do not use any type of comments. I would recommend putting at least a small comment above each $.each call to explain in a few words what that part of the code does. Scanning over the comments will give a better intuitive understanding of what the code actually does.

Logic errors

Note: You are working with floating point numbers. Repeated operations on floating point numbers can cause inaccuracies.

Your second loop is playing with a percentage. You likely expect the sum to be equal to the total after the last element, but since you are doing repeated additions of floating point percentages this is not guaranteed. See below for more details.

Unnecessary computing

Your second loop does some unnecessary multiplication. You seem to want that obj.value becomes the sum of all values before and including that item. The way you get there is difficult. If you simply keep track of the sum up until that point, you don't have to do multiplication at all. You just calculate the percentage for that item, and add the current value to the sum so far. This will also guarantee that the sum is actually equal to the total you calculated previously. (This also shows that Roamer-1888's observation that you can use .reverse() instead is correct. The value of the current item is guaranteed to be >= the value of the previous item if the value of each item is guaranteed to be >= 0. Since you calculate percentages, I expect this to be true.)

You assign the original value to obj.original, but you never use it anymore. There is no reason to put this there. It is also not descriptive of what it contains.

You destroy the data object in such a way that executing the code twice will yield different results. Why not reduce the original datastructure until you have what you wanted?

data.reduce(function ( out, current ) {
  return out.concat( current.data );
}, []);
\$\endgroup\$
  • \$\begingroup\$ Please look at the comment I posted under the @Roamer-1888 answer. \$\endgroup\$ – cFreed Dec 31 '15 at 23:06

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.