Skip to main content
Code Review

Return to Answer

added 3 characters in body
Source Link
konijn
  • 34.3k
  • 5
  • 71
  • 267
  • Q1: Sure
  • Q2: Technically you are not setting a name, and that variable is hence misleading
  • Q3: You could do that, but I would propose you just send the whole object to the constructor, and let that constructor do the .. constructing ;)

Further notes:

  • g is a terrible name, Group is a much better one (capital G because this function is a constructor)
  • Using the jQuery each feels wrong, just go bare metal and use the Array built-ins
  • Speaking of which getGroupsWithStatus becomes a one-liner with the Array.filter() built-in
  • The reliance of main() on myGroups is unhealthy, you are treating myGroups as a global, it should be local to main, especially if you are putting querying functions within main
  • If you are not using a fat arrow syntax function, then you should provide a function name (getGroupsWithStatus = function (status) => getGroupsWithStatus = function queryGroupsWithStatus(status)

This is my counter proposal:

var jsonTmp = '{ "groups": [ { "name": "G1", "status": "inactive" }, { "name": "G2", "status": "active"}, { "name": "G3", "status": "inactive" }] }'
var dataTmp = JSON.parse(jsonTmp)
function Group(key, properties) {
 this.key = key;
 
 properties = properties || {};
 this.name = properties.name;
 this.status = properties.status; 
}
​
function main() {
 this.groups = [];
​
 this.createGroups = function (json) {
 var data = JSON.parse(json)
 this.groups = dataTmpdata.groups.map( (o,key) => new Group(key,o) ); 
 }
​
 this.getGroupsWithStatus = function querygetGroupsWithStatus(status) {
 return this.groups.filter( group => group.status == status )
 }
}
​
var myGame = new main();
myGame.createGroups(jsonTmp);
console.log( myGame.getGroupsWithStatus("inactive") ); 

  • Q1: Sure
  • Q2: Technically you are not setting a name, and that variable is hence misleading
  • Q3: You could do that, but I would propose you just send the whole object to the constructor, and let that constructor do the .. constructing ;)

Further notes:

  • g is a terrible name, Group is a much better one (capital G because this function is a constructor)
  • Using the jQuery each feels wrong, just go bare metal and use the Array built-ins
  • Speaking of which getGroupsWithStatus becomes a one-liner with the Array.filter() built-in
  • The reliance of main() on myGroups is unhealthy, you are treating myGroups as a global, it should be local to main, especially if you are putting querying functions within main
  • If you are not using a fat arrow syntax function, then you should provide a function name (getGroupsWithStatus = function (status) => getGroupsWithStatus = function queryGroupsWithStatus(status)

This is my counter proposal:

var jsonTmp = '{ "groups": [ { "name": "G1", "status": "inactive" }, { "name": "G2", "status": "active"}, { "name": "G3", "status": "inactive" }] }'
var dataTmp = JSON.parse(jsonTmp)
function Group(key, properties) {
 this.key = key;
 
 properties = properties || {};
 this.name = properties.name;
 this.status = properties.status; 
}
​
function main() {
 this.groups = [];
​
 this.createGroups = function () {
 this.groups = dataTmp.groups.map( (o,key) => new Group(key,o) ); 
 }
​
 this.getGroupsWithStatus = function querygetGroupsWithStatus(status) {
 return this.groups.filter( group => group.status == status )
 }
}
​
var myGame = new main();
myGame.createGroups();
console.log( myGame.getGroupsWithStatus("inactive") ); 

  • Q1: Sure
  • Q2: Technically you are not setting a name, and that variable is hence misleading
  • Q3: You could do that, but I would propose you just send the whole object to the constructor, and let that constructor do the .. constructing ;)

Further notes:

  • g is a terrible name, Group is a much better one (capital G because this function is a constructor)
  • Using the jQuery each feels wrong, just go bare metal and use the Array built-ins
  • Speaking of which getGroupsWithStatus becomes a one-liner with the Array.filter() built-in
  • The reliance of main() on myGroups is unhealthy, you are treating myGroups as a global, it should be local to main, especially if you are putting querying functions within main
  • If you are not using a fat arrow syntax function, then you should provide a function name (getGroupsWithStatus = function (status) => getGroupsWithStatus = function queryGroupsWithStatus(status)

This is my counter proposal:

var jsonTmp = '{ "groups": [ { "name": "G1", "status": "inactive" }, { "name": "G2", "status": "active"}, { "name": "G3", "status": "inactive" }] }'
function Group(key, properties) {
 this.key = key;
 
 properties = properties || {};
 this.name = properties.name;
 this.status = properties.status; 
}
​
function main() {
 this.groups = [];
​
 this.createGroups = function (json) {
 var data = JSON.parse(json)
 this.groups = data.groups.map( (o,key) => new Group(key,o) ); 
 }
​
 this.getGroupsWithStatus = function querygetGroupsWithStatus(status) {
 return this.groups.filter( group => group.status == status )
 }
}
​
var myGame = new main();
myGame.createGroups(jsonTmp);
console.log( myGame.getGroupsWithStatus("inactive") ); 

Source Link
konijn
  • 34.3k
  • 5
  • 71
  • 267
  • Q1: Sure
  • Q2: Technically you are not setting a name, and that variable is hence misleading
  • Q3: You could do that, but I would propose you just send the whole object to the constructor, and let that constructor do the .. constructing ;)

Further notes:

  • g is a terrible name, Group is a much better one (capital G because this function is a constructor)
  • Using the jQuery each feels wrong, just go bare metal and use the Array built-ins
  • Speaking of which getGroupsWithStatus becomes a one-liner with the Array.filter() built-in
  • The reliance of main() on myGroups is unhealthy, you are treating myGroups as a global, it should be local to main, especially if you are putting querying functions within main
  • If you are not using a fat arrow syntax function, then you should provide a function name (getGroupsWithStatus = function (status) => getGroupsWithStatus = function queryGroupsWithStatus(status)

This is my counter proposal:

var jsonTmp = '{ "groups": [ { "name": "G1", "status": "inactive" }, { "name": "G2", "status": "active"}, { "name": "G3", "status": "inactive" }] }'
var dataTmp = JSON.parse(jsonTmp)
​
function Group(key, properties) {
 this.key = key;
 
 properties = properties || {};
 this.name = properties.name;
 this.status = properties.status; 
}
​
function main() {
 this.groups = [];
​
 this.createGroups = function () {
 this.groups = dataTmp.groups.map( (o,key) => new Group(key,o) ); 
 }
​
 this.getGroupsWithStatus = function querygetGroupsWithStatus(status) {
 return this.groups.filter( group => group.status == status )
 }
}
​
var myGame = new main();
myGame.createGroups();
console.log( myGame.getGroupsWithStatus("inactive") ); 

default

AltStyle によって変換されたページ (->オリジナル) /