Gfrktyl

I'm trying to get better at javascript and have built a simple function that returns 1 if the mode is equal to the mean, and 0 if not. You can enter an array of numbers into the function:

// tests

meanMode([1,2,3]); - returns 0

meanMode([4,4,4,6,2]); - returns 1

meanMode([4,4,4,6,6,6,2]); - returns 0

To clarify, the mean is the middle number in the array, and mode most occurring number.

When I was writing this, I wondered whether I create too many variables and overcomplicate things. The problems I thought I might be making are:

1. There are too many variables


2. It's confusing and over complex


3. It can be hard to retrace my logic and know what the code does and why it's there

I have a few questions that I hope better coders than myself can answer, as I'm relatively new to javascript:

1. Was using a counts variable necessary?


2. Are all variable keywords (const & let) optimal?


3. Is it wise to include comments to describe what the code does?


4. Was declaring the mode variable before assigning its value the best way? Its value depends on a condition

Before writing this I wrote a brief logic plan as follows:

Mode:

1. var uniqueVals with unique vals (set)


2. for each unique value (uniqueVals.map), count no of occurrences in arr (filter.length) & store as an array


3. if all values are identical, return 0, otherwise get the most frequent & store in variable (there may be cases with no mode)

Mean:

1. sum all numbers


2. divide by no of numbers

And:

• if unique values length is same as arr length, return 0


• if mean = mode, return 1

When people write programming, is it usually best to have a detailed plan to follow? I wonder whether there are any best practices that I could follow to improve my coding ability.

function meanMode(arr) {

    let uniqueVals = Array.from(new Set(arr));

    let counts = uniqueVals.map(function (c, i, a) {

        return arr.filter(function (c2) {

            return c2 == c

        }).length

    }); // [3,1,1]



    if (arr.every(sameValues)) {

        return 0;

    } else {

        var mode;



        // if highest number in counts appears just once, then there is a mode

        if ((counts.filter(function (x) { return x == (Math.max(...counts)) }).length) == 1) {

            mode = uniqueVals[counts.indexOf(Math.max(...counts))]; // scope issue?

        } else {

            return 0;

        }



        const mean = (arr.reduce(function (a, c) { return a + c })) / (arr.length);



        if (mode == mean) {

            return 1;

        } else {

            return 0;

        }

    }



    function sameValues(c, i, a) {

        return c == a[0];

    }

}

First of all, I don’t think this is reasonable:

if unique values length is same as arr length, return 0

If your array consists of unique values only (e.g. [1, 2, 3, 4]), its mode is not unique, rather there is multiple modes (in this case [1, 2, 3, 4]). How you define what mode exactly means depends on your application so I’m going to ignore this for now.

Let’s look at your code. I’ll list some observations:

• 
  Function name. A name like meanEqualsMode would more clearly describe what the function does


• 
  Unused variables. There are a few unused variables. Line 3 uses three arguments for the map function’s callback, but the callback is only ever called with one argument: The current array element. You also never use i and a.


• 
  Single-letter variable names. This style of naming problematic for a variety of reasons. Most importantly, it’s harder to reason about your own code. For example, in line 21, the reduce function has a and c as callback arguments. sum and count would be better names.


• 
  Separation of concerns. Your meanMode function does multiple things. It calculates the mode and the mean. Instead, use separate functions to calculate the mode and mean separately. If your applications uses the check for equality of mode and mean a lot, this would be a third function calling the other two. I’ve done that in the updated code below.


• 
  Predictable results. You state that meanMode should return whether the mode and the mean are equal; however, it actually returns a Number (1 or 0). A comparison function (e.g. a function containing the words is, equal, or similar) should always return a boolean value (true or false). This again makes it easier to reason about your code. Just reading meanEqualsMode([2, 3, 5]) should tell you what the result of that function will be without the need to look at the actual implementation.

function meanEqualsMode(array) {

  const mode = arrayMode(array);

  const mean = arrayMean(array);



  return mode === mean;

}



function arrayMode(array) {

  const frequencies = new Map();



  for (const value of array) {

    const currentCount = frequencies.has(value) ? frequencies.get(value) : 0;

    frequencies.set(value, currentCount + 1);

  }



  let highestCount = 0;

  let mostOccurringValue;



  for (const [value, count] of frequencies) {

    if (count >= highestCount) {

      highestCount = count;

      mostOccurringValue = value;

    }

  }



  return mostOccurringValue;

}



function arrayMean(array) {

  return array.reduce((sum, value) => sum + value) / array.length;

}

Now, answering some of your questions:

Was using a counts variable necessary?

Yes and no. In order to calculate the mode of a list of numbers, you need to know the number of occurrences for each number. However, it is possible to determine the mode while counting the occurences; thus, allowing you to calculate the mode without explicitly storing the occurences. To keep the code simple, I opted for not combining these steps.

Are all variable keywords (const & let) optimal?

Probably not. You are using const and let. There is very, very little reason to also still use var at all. Most variables are assigned only once and can be defined using const. Some variables have to be declared using let because they’re (re-)assigned later.

Is it wise to include comments to describe what the code does?

Yes. Take my code for example. Is there something you don’t understand? Then it could’ve maybe explained with a comment. You used a reduce function in your code which I often find hard to read. You need to ask yourself: Is my code dealing with a complex problem that needs explaining or is it just written in a hardly readable fashion? A complex problem is best explained with comments. Code which is hard to read and thus hard to reason about is better fixed by making it more readable and obvious.

Was declaring the mode variable before assigning its value the best way? Its value depends on a condition

No, it wasn’t. The mode of a list of numbers always exists although it might not be unique. Therefor its value does not logically depend on a condition.

Too complex

As the issue of complexity has not come up and as the functions complexity is O(n²) for a problem that can be solved in O(n) where n is length of arr, I must point out some of the flaws.

The code comments show all the iterations for a worst case argument [2,4,6,8,12,14,16,18,10,10] the return will be 1. (** is exponential operator)

function meanMode(arr) {

    let uniqueVals = Array.from(new Set(arr));        // << array iteration * 2

    let counts = uniqueVals.map(function (c, i, a) {  // << outer iteration

            return arr.filter(function (c2) {         // << inner iteration **2

                return c2 == c

            }).length

        });

    if (arr.every(sameValues)) {                      // << array iteration

        return 0;

    } else {

        var mode;

        if ((counts.filter(function (x) {             // << outer iteration

                    return x == (Math.max(...counts)) // << inner iteration **2 to get max

                }).length) == 1) {



            // Next line has nested iteration **2 to get max again

            mode = uniqueVals[counts.indexOf(Math.max(...counts))]; 

        } else {

            return 0;

        }

        const mean = (arr.reduce(function (a, c) {    // << array iteration

                return a + c

            })) / (arr.length);



        if (mode == mean) {

            return 1;

        } else {

            return 0;

        }

    }

    function sameValues(c, i, a) {

        return c == a[0];

    }

}

Not only are the 3 instances of iteration at O(n²) unneeded, you use Array.filter to count!! :( ... Use Array.reduce to count so you don't need to allocate memory each time you add 1.

There are also repeated iterations that compute the same value. Math.max(...counts) requires one full iteration of counts each time it is called and you call it in the worst case 2n times.

Even when you add these optimisations (Array.reduce rather than Array.filter and calculate max once) and gain about 50% performance you are still at O(n²).

In a single pass O(n)

Using a counts = new Map(); and a single iteration you can compute the sum to get the mean (after iteration), count each items frequency (via the map), and as you count the frequency you can keep a record of the top two max counts and the current max mode.

After the iteration compute the mean from the sum divide the arr.length, check if mode is equal to mean and if it is check that the top two frequencies are not equal to return 1 (or true).

An example of O(n) solution tuned for performance. (has optimisation that give an advantage if there are many sequences of the same value)

function isMeanMode(arr) {

    const counts = new Map();

    var i, prevMaxVal, item, maxA = -1, maxB = -1, index = 0, max = 1, sum = 0;

    for (i = 0; i < arr.length; i ++) {  // Only one iteration O(n)

        const val = arr[i];

        const item = counts.get(val);

        sum += val;

        if (item) { 

            item.count++;

            while (val === arr[i + 1]) { // for quick count of sequences

                i++; 

                item.count++;

                sum += val;

            }

            if (max <= item.count) {

                max = item.count;

                if (prevMaxVal !== val) { // tracks the top two counts

                    maxB = maxA;          // if maxA and maxB are the same

                    maxA = max;           // after iteration then 2 or more mode vals

                } else { maxA = max }

                index = i;

                prevMaxVal = val;

            }



        } else { counts.set(val, {count : 1}) }

    }

    if (counts.size === arr.length || maxB !== -1 && maxA === maxB) { return 0 }

    return arr[index] === sum / arr.length ? 1 : 0;

}

The above solution has a performance increase from 2 orders of magnitude + faster for large random arrays, to 3-4 times faster for very small arrays.