7
\$\begingroup\$

I am a beginner in JavaScript and just wanted to get my code reviewed. Here I have created a function that takes in an array and returns the mode. If there is no more then I just want the function to return the first number. It seems a little too long and I feel like there may be more clever ways to do it. Again, keep in mind that I am a beginner, so I don't have a huge toolbox.

var mode = function(arr){
    var numMapping = {};
    for(var i = 0; i < arr.length; i++){
        if(numMapping[arr[i]] === undefined){
            numMapping[arr[i]] = 0;
        }        
            numMapping[arr[i]] += 1;
    }
    var greatestFreq = 0;
    var mode;
    for(var prop in numMapping){
        if(numMapping[prop] > greatestFreq){
            greatestFreq = numMapping[prop];
            mode = prop;
        }
    }
    return parseInt(mode);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Mode (Average) from Rosetta Code: rosettacode.org/wiki/Averages/Mode#JavaScript \$\endgroup\$ – MT0 Oct 30 '14 at 0:40
  • \$\begingroup\$ You don't have to do the second part of your code(finding the mode) after looping the array once already - you can do it during the mapping \$\endgroup\$ – Sacho Oct 30 '14 at 7:02
  • \$\begingroup\$ Worth noting that there can be more than one mode in an array. Your code will just return the first-occurring. \$\endgroup\$ – Stuart Oct 30 '14 at 19:11
7
\$\begingroup\$
var mode = function(arr){

It's nice to name your functions for debugging purposes, if none other. You could replace this with a function declaration, e.g. function mode(arr) { or just name the function expression: var mode = function mode(arr) {

    var numMapping = {};
    for(var i = 0; i < arr.length; i++){
        if(numMapping[arr[i]] === undefined){
            numMapping[arr[i]] = 0;
        }        
            numMapping[arr[i]] += 1;
    }

I would use arr.forEach(), since it lets you operate on the elements directly. You could also make the mapping a one-liner(but both of these things are more nitpicks/personal style choice)

    var greatestFreq = 0;
    var mode;
    for(var prop in numMapping){
        if(numMapping[prop] > greatestFreq){
            greatestFreq = numMapping[prop];
            mode = prop;
        }
    }
    return parseInt(mode);
}

You can do this whole part during your first loop! Also, parseInt, while safe in this case, is not generally safe to use without a radix(you'd have linters complaining about this usage here). You could use parseInt(mode, 10), or some ToNumber() transformation like +mode(http://es5.github.io/#x11.4.6).

Here's a revised example:

var mode = function mode(arr) {
    var numMapping = {};
    var greatestFreq = 0;
    var mode;
    arr.forEach(function findMode(number) {
        numMapping[number] = (numMapping[number] || 0) + 1;

        if (greatestFreq < numMapping[number]) {
            greatestFreq = numMapping[number];
            mode = number;
        }
    });
    return +mode;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Unless you are golfing you should avoid stuff like +mode. While it's generally understood it is not as clear as parseInt. In your code there is no need to have var mode = function[...]. Either use var mode = or a function name, using both is superfluous. Also, having an inner and outer variable share the same name is generally bad. In this case mode is the name used for the function and inner variable. \$\endgroup\$ – Annan Jun 7 '18 at 8:08
4
\$\begingroup\$

As others have said, this is reasonable beginner's code. And @Sacho's answer gives good suggestions for improving it.

Below is a solution using some slightly more advanced techniques, if you're interested:

var mode = function mode(arr) {
    return arr.reduce(function(current, item) {
        var val = current.numMapping[item] = (current.numMapping[item] || 0) + 1;
        if (val > current.greatestFreq) {
            current.greatestFreq = val;
            current.mode = item;
        }
        return current;
    }, {mode: null, greatestFreq: -Infinity, numMapping: {}}).mode;
};

console.log(mode([3, 1, 4, 1, 5, 9, 2, 6, 5, 3, 5, 8, 9, 7, 9, 3, 2, 3, 8, 4, 6]));
//=> 3

To see how it works, if you drop off the .mode at the very end of the return, you will get this object instead:

{
    "mode": 3,
    "greatestFreq": 4,
    "numMapping": {"1": 2, "2": 2, "3": 4, "4": 2,
                   "5": 3, "6": 2, "7": 1, "8": 2, "9": 3}
}

This object is built up a piece at a time by the reduce call running over each element of the array, updating the numMapping each time and the greatestFreq and mode when appropriate.

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

This is not bad at all. A few things can be simplified a bit.


When checking if arr[i] exists in numMapping:

if(numMapping[arr[i]] === undefined){

A simpler and more natural way is this:

if (!(arr[i] in numMapping)) {

As a tiny optimization, instead of this:

if (!(arr[i] in numMapping)) {
    numMapping[arr[i]] = 0;
}
numMapping[arr[i]] += 1;

This is somewhat better:

if (!(arr[i] in numMapping)) {
    numMapping[arr[i]] = 1;
} else {
    numMapping[arr[i]] += 1;
}

Because this way you save one extra lookup of arr[...] and also numMapping[...], by directly assigning 1 instead of 0 and then incrementing later.


At a few places the indentation was off, and I would recommend a bit more generous spaces in if(...){ and for(...){ statements, use if (...) { and for (...) { instead (like in my examples above).

\$\endgroup\$
  • 3
    \$\begingroup\$ Using for (var i in arr) is not always a good idea. var arr = [1,2,3]; arr.test = "test"; for( var i in arr){ console.log( i, arr[i]); } will log 4 things but the array length is only 3. \$\endgroup\$ – MT0 Oct 30 '14 at 0:27
  • 1
    \$\begingroup\$ That's fairly contrived though(you wouldn't really want to set properties on your simple arrays, that would in most cases be a misuse of them). I still agree with your comment, since you can use arr.forEach(function (element) { }), and reduce the further arr[i] usage to just element \$\endgroup\$ – Sacho Oct 30 '14 at 7:01
  • 1
    \$\begingroup\$ In JavaScript for (var i in arr) is hugely discouraged; look at stackoverflow.com/questions/500504/… What @MT0 wrote appears to be rather contrived. \$\endgroup\$ – wei2912 Oct 30 '14 at 8:43
  • 1
    \$\begingroup\$ @wei2912 absolutely right, and I know it, I don't know what drove me to write that... Deleted now, and asked the OP to accept Sacho's instead. \$\endgroup\$ – janos Oct 30 '14 at 8:55
1
\$\begingroup\$

You might appreciate taking advantage of some ES6 features. You're using an object literal like a map, so why not use a real Map? Speaking of ES6, since you're a beginner, you would benefit from taking the time to learn it now and save yourself the headache later. As others have noted, you can accomplish finding the mode in a single for loop.

const mode = (array) => {
  const map = new Map();
  let maxFreq = 0;
  let mode;

  for(const item of array) {
    let freq = map.has(item) ? map.get(item) : 0;
    freq++;

    if(freq > maxFreq) {
      maxFreq = freq;
      mode = item;
    }
    
    map.set(item, freq);
  }

  return mode;
};

const testArray = [1, 1, 2, 3, 5, 8, 13];
console.log(`Mode of [${testArray}] is ${mode(testArray)}.`);

\$\endgroup\$

protected by Jamal Dec 7 '18 at 4:14

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.