Get average of each nested array in dynamic object

Question

I'm iterating this JSON using a for loop:

var movieObj = {
 "cat": {
 "moveName": "Breath",
 "rating": [4, 5, 4, 3]
 },
 "cat1": {
 "moveName": "shallow",
 "rating": [5, 5, 5, 5, 4]
 }
}

Is there any way to improve this code?

var sum;
Array.prototype.getSum = function (ele) {
 sum = ele.reduce(function (a, b) {
 return a + b
 });
 return sum / ele.length
}
for (var key in movieObj) {
 var b = movieObj[key];
 if (Array.prototype.getSum(b['rating']) < 5) {
 console.log(b['moveName']);
 }
}

• pacmaninbw: The title of the question indicates the use of opinion, specifically "what is the best way to write the blow code", the rest of the question is fine. Is there some way to could change the title? Please see this webpage on how to ask a good question codereview.stackexchange.com/help/how-to-ask.

Answer 1

First off, you should ensure that each key is a properly defined member of movieObj, not part of the prototype:

for (var key in movieObj) {
 if (movieObj.hasOwnProperty(key)) {
 // ...
 }
}

Instead of declaring getSum on Array, you can use the builtin reduce function:

var sum = function(previous, current) {
 return previous + current;
};
var ratings = movieObj[key].rating;
if (ratings.reduce(sum) / ratings.length < 5) {
 // ...
}

Finally, your console.log statement can use direct property access, instead of square bracket notation. I believe moveName is a typo, and should in fact be movieName, but correct me if I'm wrong there:

console.log(movieObj[key].movieName);

Putting it all together gives:

var sum = function(previous, current) {
 return previous + current;
};
for (var key in movieObj) {
 if (movieObj.hasOwnProperty(key)) {
 var ratings = movieObj[key].rating;
 if (ratings.reduce(sum) / ratings.length < 5) {
 console.log(movieObj[key].movieName);
 }
 }
}

Answer 2

Global Variables

In current code, the variables movieObj, sum, etc. are global. Global variables are bad and can create problems when the variable names collide with other developer's namespaces. This will be very difficult to debug in moderate-size projects.

See Why are global variables considered bad practice?

To make the variables and methods private, use Module pattern.

Naming Conventions

Use the descriptive variable names such that it'll give the idea of what it may contain, what is the purpose of it.

Example, the variable b in the for...in can be renamed to val as it is the value of a key or movie as the object stores the information of a movie.

Read Clean Code second chapter "Meaningful Names".

Overriding the prototype method

Always check if a particular method exists on the prototype. If it does not exists, then only add one. Also, regarding the name of method, I'll name the method as sum instead of getSum. Chances are, there will be method with same name and functionality in the future versions of ECMAScript and the code doesn't have to be changed.

if (!Array.prototype.sum) {
 Array.prototype.sum = function () {
 // Do something
 };
}

for...in vs Object.keys() + forEach

for...in to iterate over object is slower^Benchmark . Use Object.keys() to get the list of all keys in that object and use Array#forEach on that array to iterate over it.

Object.keys(obj).forEach(function (key) {
 // ...
});

this inside method defined on prototype

this inside the method on prototype refers to the variable on which the method is called. Example, when calling array.push, this inside the push() will refer to the array on which the method is called. So, there is no need to pass the array as parameter to the method.

Array.prototype.sum = function () {
 console.log(this);
 // Use `this` to refer to array
};

Bracket notation vs Dot notation

When the keys are known and are static and does not contain special characters, use the dot notation to access the value of that key in the object. Dot notation is faster and clear to read.

Instead of obj['moveName'], use obj.moveName.

See JavaScript property access: dot notation vs. brackets?

Typo

In the object, the keys for movie name has typo. It should be movieName.

ECMAScript 2015

If the target environment support latest version of JavaScript, use the features provided by it.

Complete code:

if (!Array.prototype.sum) {
 Array.prototype.sum = function () {
 var sum = this.reduce(function (a, b) {
 return a + b;
 }, 0);
 return sum / this.length;
 };
}
(function () {
 'use strict';
 var movieObj = {
 cat: {
 movieName: 'Breath',
 rating: [4, 5, 4, 3]
 },
 cat1: {
 movieName: 'shallow',
 rating: [5, 5, 5, 5, 4]
 }
 };
 Object.keys(movieObj).forEach(function (key) {
 var movie = movieObj[key];
 if (movie.rating.sum() < 5) {
 console.log(movie.movieName);
 }
 });
}());

Answer 3

Your Array.prototype.getSum() is awkward and against the beautiful principles of JS's prototypical structure. Since it will reside in the prototype of all arrays to be defined it shall be invoked by the array itself. No need to pass an argument.

In test code i would agree with Tushar's correction but in production you can not and should not simply extend the Array.prototype like that. You should use Object.defineProperty() tool and make this extension a non enumerable property.

I also believe that the function could have been named more involvedly like Array.prototype.average() since you are not getting sum but the average. Also you might simplify Array.prototype.average() and the rest of the code as follows;

Object.defineProperty(Array.prototype,"average",{ value: function(){
 return this.reduce((p,c) => p += c/this.length,0);
 }
 });
var movieObj = {
 "cat": {
 "moveName": "Breath",
 "rating": [4, 5, 4, 3]
 },
 "cat1": {
 "moveName": "shallow",
 "rating": [5, 5, 5, 5, 4]
 }
};
Object.keys(movieObj).forEach(k => movieObj[k].rating.average() < 5 && console.log(movieObj[k].moveName));