2
\$\begingroup\$

I don't really ever write JS like this in my real workflow, but I was just trying to answer a question for someone - that was then deleted.

I wrote the following and I'd like to know if this is close to smart / or how I can improve on these patterns. I'm interested in prototypes but also functional approaches. I am pretty familiar with es2015 - so if you please, that isn't the kinda of thing I'm trying to learn in this case - so please stick with 'var' etc.

jsFiddle

"use strict";

console.clear(); // just to keep things clean in the console... not for production

function Game(newGameTitle) {
  this.title = newGameTitle;
  this.players = [];
  this.createPlayer = createPlayer;
  this.findPlayer = findPlayer;
  this.listPlayers = listPlayers;
  console.log('Game "', newGameTitle, '" created.');
}

function createPlayer(newPlayerName) { // assigned on line 8
  console.log('player', newPlayerName, 'created.');
  // get this player into the array somehow...
  var newPlayerObject = {
    name: newPlayerName,
    level: 1, // reasonable default?
  };
  // you could use a constructor here like I did with game to create a prototype or something too
  // then you can have Player.methods to 'level up' etc. - but the point should be made... you'd use the new keyword here in that case
  this.players.push(newPlayerObject);
  // 'this' is going to refer to the object that eventually 'calls' this... so the 'game' hopScotch
  // same goes for all it's uses
  console.log(this); // show the changes to the game
  return this; // in case you want to chain? not sure...
}

function findPlayer(playerName) { // assigned on line 9
  console.log('found', playerName);
  function isPlayer(targetName) {
    return targetName = name;
  }
  return this.players.find(isPlayer);
}

function listPlayers() { // assigned on line 10
  console.log('Player list:');
  this.players.forEach( function(currentPlayer, currentIndexInArray, fullArray /*if needed*/) {
    console.log(currentIndexInArray, currentPlayer);
  });
  return this;
  // I guess this doesn't have any side-effects - but what if I wanted to chain it?
}

// create 'game'
var hopScotch = new Game('Hopscotch');
console.log(hopScotch);

// add some players
hopScotch.createPlayer('@sheriffderek');
hopScotch.createPlayer('@ivy');
hopScotch.createPlayer('@valentineRose');

// try some methods
hopScotch.findPlayer('@sheriffderek');
hopScotch.listPlayers();

// checking chaining
hopScotch.createPlayer('@h2whoah').findPlayer('@h2whoah');
hopScotch.listPlayers().findPlayer('@ivy');
\$\endgroup\$
4
  • \$\begingroup\$ I'd strongly suggest to rewrite all of this in ES6/ES7. If you put in enough effort at the beginning to learn all the new stuff, you'll truly thank yourself later as the code will have less boilerplate, the syntax will make complete sense (using class and class methods (specifically, withclass fields implementation), etc.) and so on. \$\endgroup\$
    – Denialos
    Aug 27, 2017 at 6:01
  • \$\begingroup\$ Thanks for the thoughts Denianlos. I use es2015 and classes in Ember every day, but I'm trying to learn some basic stuff from the past - \$\endgroup\$ Aug 27, 2017 at 16:51
  • \$\begingroup\$ Also something to note, you aren't using prototypes at all. You are assigning methods directly to the object. \$\endgroup\$
    – Gerrit0
    Aug 27, 2017 at 17:21
  • \$\begingroup\$ @Gerrit0 - ah ha. Thanks. I remember how that works now. \$\endgroup\$ Aug 27, 2017 at 17:41

1 Answer 1

1
\$\begingroup\$

Bug

In this code, what is the return targetName = name line trying to achieve?

function findPlayer(playerName) { // assigned on line 9
  console.log('found', playerName);
  function isPlayer(targetName) {
    return targetName = name;
  }
  return this.players.find(isPlayer);
}

name is undefined, and probably you wanted to use == instead of =.

Also, the function should not print found ..., before actually finding anything.

Code organization

The functions that should be called on Game instances should not be in the global namespace. Not only they pollute the global namespace, they are not useful and misleading without a Game instance.

Also, unless you want to allow manipulating the players array directly, it would be better to hide it from users.

Printing inside functions is a bad practice unless it is their dedicated purpose.

This would be a better organization:

function createGame(title) {
  var players = [];

  function addPlayer(name) {
    players.push({ name: name });
  }

  function findPlayer(name) {
    return players.filter(function(player) { return player.name === name; });
  }

  function printPlayers() {
    console.log('Player list:');
    players.forEach(function(player, index) {
      console.log(index, player);
    });
  }

  return {
    title: title,
    addPlayer: addPlayer,
    findPlayer: findPlayer,
    printPlayers: printPlayers
  };
}

"just-in-case" programming

You made it possible to chain calls to createPlayer, along with this comment:

return this; // in case you want to chain? not sure...

When you're not sure to implement something, there's a really simple rule of thumb you can follow: don't do it. Everything in the code should have a purpose, avoid writing code "just in case it might be useful someday".

\$\endgroup\$
3
  • \$\begingroup\$ This is great. Thank you. When you say printing - do you mean, the console.logging? \$\endgroup\$ Aug 27, 2017 at 17:40
  • \$\begingroup\$ @sheriffderek yes \$\endgroup\$
    – janos
    Aug 27, 2017 at 17:40
  • \$\begingroup\$ OK. Cool. Yeah, I wouldn't use those / but they are just in place of the UI for this example. \$\endgroup\$ Aug 27, 2017 at 17:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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