Game prototype with methods to manage players

Question

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');

Answer 1

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".