TransWikia.com

MMMRjs a product of BYTES Genesis

Code Review Asked by Syed Mohammad Sannan on November 17, 2021

We have published a product today called MMMR.js this product is licensed under the MIT license and is built by Syed Mohammad Sannan.

The uniqueness of this product is that it is built with and for Javascript though there are many libraries out there such as NumPY, SciPY, and much much more but they are built for Python or another language and there isn’t one for JavaScript that is where MMMR.js comes.

here.

This is the source code of MMMR.js:

// Mean - The average value.
// Median - The mid point value.
// Mode - The most common value.
// Range The smallest and the biggest value.

// This is Mean.
// Mean returns the average value from an array of numbers.
// To call/run this function we can type mean(Your array of numbers separated with commas ',');
const mean = (numbers) => {
  let total = 0,
    i;
  for (i = 0; i < numbers.length; i += 1) {
    total += numbers[i];
  }
  return total / numbers.length;
};



// This is Median.
// Median returns the middle point value from an array of numbers.
// To call/run this function we can type median(Your array of numbers separated with commas ',');
const median = (numbers) => {
  const sorted = numbers.slice().sort((a, b) => a - b);
  const middle = Math.floor(sorted.length / 2);

  if (sorted.length % 2 === 0) {
    return (sorted[middle - 1] + sorted[middle]) / 2;
  }

  return sorted[middle];
};



// This is Mode.
// Mode returns the most common/used value from an array of string, numbers, etc.
// To call/run this function we can type mode(your array of numbers);
const mode = (a) => {
  a.sort((x, y) => x - y);

  let bestStreak = 1;
  let bestElem = a[0];
  let currentStreak = 1;
  let currentElem = a[0];

  for (let i = 1; i < a.length; i++) {
    if (a[i - 1] !== a[i]) {
      if (currentStreak > bestStreak) {
        bestStreak = currentStreak;
        bestElem = currentElem;
      }

      currentStreak = 0;
      currentElem = a[i];
    }

    currentStreak++;
  }

  return currentStreak > bestStreak ? currentElem : bestElem;
};



// This is Range.
// Range is basically a function used to return the smallest and the biggest values from an array of numbers.
// To call/run this function we can type range(your array of numbers);
const range = (value) => {
  value.sort();
  return [value[0], value[value.length - 1]];
}

We just wanted you to review this code and tell us is this library user-friendly, maintainable and what improvements can we add to this.

3 Answers

Not an O(n) MMMR library

There are a lot unnecessary inefficiency on that code. For big data, it makes a lot difference.

The current version, time complexity-wise... $$text{ mean() and mode() are }O(n)$$. $$text{range() and median() are } O(n .log(n))$$.

All could be implemented in average time complexity O(n). Not only that, checking if the list is ordered, taking the mean, max, min and mode could be done with one loop.

Once, you have checked the list is ordered, the median can be calculated as in this code. If is not, you can use "Median find algorithm": https://rcoh.me/posts/linear-time-median-finding/

Just as example, the code below shows how to detect ordering, and how to find the max, the min and the mean of a list. Now, you can implement "Median Finding" and add the mode calculation to have an efficient MMMR library.


const medianFromSorted = (numbers) => {
  const middle = Math.floor(numbers.length / 2);
  if (numbers.length % 2 === 0) {
    return (numbers[middle - 1] + numbers[middle]) / 2;
  }
  return numbers[middle];
};


const mmr = (numbers) => {

  if (numbers.length === 0) {
    return {mean: undefined, moda: undefined, median: undefined, range: undefined};
  }
  if (numbers.length === 1) {
    return {mean: numbers[0], moda: numbers[0], median: numbers[0], range: [numbers[0], numbers[0]]};
  }
  let total = 0, i;
  let asc = numbers[0] <= numbers[1];
  let desc = numbers[0] >= numbers[1];
  let min = numbers[0];
  let max = numbers[0];
  let previous = numbers[0];

  for (i = 0; i < numbers.length; i++) {
    total += numbers[i];
    if (numbers[i] < min)
      min = numbers[i];
    if (numbers[i] > max)
      max = numbers[i];
    asc = asc && (previous <= numbers[i]);
    desc = desc && (previous >= numbers[i]);
    previous = numbers[i];
  }
  let median;
  if (asc || desc)
    median = medianFromSorted(numbers);
  else
    median = medianFinding(numbers);

  return {
    mean: total / numbers.length,
    median: median,
    range: [min, max]
  }

};
```

Answered by rdllopes on November 17, 2021

Rotara already mentioned some great points - the comments can definitely be improved and follow common conventions, sorting should happen on copies of the data instead of the original data, and the input should be validated. Consider the case of mean when an empty array is passed in. The value of numbers.length will be 0 so this leads to division by zero, which results in NaN. Perhaps it would be wise to throw an exception in that case.

Let's look at the mean() function. It has a traditional for loop, though the iterator variable, i.e. i, is only used to dereference values in the array. That could be changed to the simpler syntax of a for...of loop

const mean = (numbers) => {
  let total = 0;

  for (const num of numbers) {
    total += num;
  }
  return total / numbers.length;
};

Moreover, a functional approach could be used with Array.reduce():

const sum = (a, b) => a + b
const mean = numbers => {
  return numbers.reduce(sum, 0) / numbers.length;
}

I tried these various functions with an array of five elements, as well as fifteen elements. It seems the original code is somewhat faster than the other approaches in FF and Chrome. So for a library that may be used with many projects it may make sense to keep with the original syntax. While I don't condone plagiarism I did peek at the source of mathjs and saw that its mean() uses a custom function deepForEach() which is basically a traditional for loop. As this article explains, the for...of loop uses an iterator method for each iteration and that is the cause of the slower execution.

As this freecodecamp article explains:

The first step in optimizing the amount of work in a loop is to minimize the number of object members and array item lookups.

You can also increase the performance of loops by reversing their order. In JavaScript, reversing a loop does result in a small performance improvement for loops, provided that you eliminate extra operations as a result.

This means two optimizations for those loops would be to do one of the following:

  • store length of the array in a separate variable instead of comparing the property each time

      for (let i = 0, const l = numbers.length; i < l; i += 1) {
    
  • add Items in reverse to eliminate steps

      // minimizing property lookups and reversing
      for (let i = numbers.length; i--; ){
    

    This could also be written as a while loop

Notice let i can be moved inside the for loop since the scope can be limited to just that block.

I also compared the mode function with the mathjs implementation for the same function. It appears that implementation uses a mapping of values to counts, which would require more memory. It also returns an array of all values that have the most number of occurrences. The MMMRjs implementation appears to return the first value that contains the maximum number of occurrences. This should be documented in the comments.

Beyond that I don't see much to suggest, except maybe unit tests to cover all scenarios that should be handled by the code. The code looks decently written, especially using const and let where appropriate. It also has consistent indentation of nesting levels.


Addition June 10, 2020

You mentioned "Most of the points mentioned in Ratora's answer were already implemented please check the updated source code by downloading the library from the link given above". I noticed that range was updated to call slice() on the input array before calling sort. However the returned array is not stored or used in the return statement. Thus the output may be incorrect if the values aren't already sorted. See the snippet below for a demonstration.

const range = (numbers) => {
  numbers.slice().sort();
  return [numbers[0], numbers[numbers.length - 1]];
};
const nums = Object.freeze([0.4, 0.2, 0.3]);
const expected = [0.2, 0.4];
const outputRange = range(nums);
console.log('output from range(): ', outputRange);
console.log('expected output: ', expected);

Answered by Sᴀᴍ Onᴇᴌᴀ on November 17, 2021

Comments

The comments such a This is Mean. are pointless.

And the comment

// To call/run this function we can type mean(Your array of numbers separated with commas ',');

is a bit strange. Are they targeting people who don't know basic JavaScript syntax? Then why mention commas, but not the brackets ([...]) needed for an array.

It probably would be a good idea to instead use a standard comment format for documentation such as jsdoc.

Sorting

The comparison function used for sorting ((a, b) => a - b) is pointless, because that is the default.

Mutating the input array

In median you are creating a copy of the input array with .slice() before sorting it, so that it isn't modified, which is a good thing. You should do the same in mode and range.

Validating the input

You should consider validating the input. mean, mode and range break or give unexpected results if you give them an empty array. It's probably best to check if they are receiving an array at all.

Answered by RoToRa on November 17, 2021

Add your own answers!

Ask a Question

Get help from others!

© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP