Pokemon game in JavaScript

Question

I'm practicing my JavaScript skills by trying to create a Pokemon game. So far, I've made the dataset and the functions required to work the game. Can someone help me optimize the code and tell me why they made that change?

function pokemon(name, hp, critical_attack, lowest_attack, highest_attack) {
    this.name = name;
    this.hp = hp;
    this.critical_attack = critical_attack;
    this.lowest_attack = lowest_attack;
    this.highest_attack = highest_attack;   
}

var Charizard = new pokemon("Charizard", 90, 30, 1, 15);
var Blastoise = new pokemon("Blastoise", 95, 28, 1, 14);
var Venasaur = new pokemon("Venasaur", 100, 26, 1, 13);

var poke_choice = new Array ("Charizard", "Blastoise", "Venasaur")

function player(type, name, pokemon) {
    this.type = type;
    this.name = name;
    this.pokemon = pokemon;
}

var user = new player("User", "Arhum", "Charizard");
console.log(window[user.pokemon].hp)

for(i = 0; i<= 2; i++) {
  if (poke_choice[i] === user.pokemon) {
    poke_choice.splice(i,1);
  }
}  

var comp_choice = function() {
  return poke_choice[Math.round(Math.random())]
}

var comp = new player("Computer","Red", comp_choice());

var user_attack = function() {
  var uhigh = window[user.pokemon].highest_attack
  var ulow = window[user.pokemon].lowest_attack
  return Math.round(Math.random().toFixed(1)*uhigh)
}        

var comp_attack = function() {
  var chigh = window[comp.pokemon].highest_attack
  var clow = window[comp.pokemon].lowest_attack
  return Math.round(Math.random().toFixed(1)*chigh)
}     

console.log(window[comp.pokemon].name)
console.log(window[user.pokemon].name)
console.log(user_attack())
console.log(comp_attack())

Answer 1

function player(type, name, pokemon) {
    this.type = type;
    this.name = name;
    this.pokemon = pokemon;
}

var user = new player("User", "Arhum", "Charizard");
console.log(window[user.pokemon].hp)

There's some really strange and bad practices going on here, especially with the pokemon field. Here, you are relying on the fact that the string passed in will correspond with some variable on the global scope that is hopefully of the type pokemon.

That's not the right way to do things. Instead, player objects should be instantiated with the actual pokemon object itself:

var user = new player("User", "Arhum", Charizard);

Then, whenever you need to access information about the player's pokemon, rather than doing this:

window[user.pokemon]

You just do this:

user.pokemon

This is a lot safer, and follows practices much better.

---

var user_attack = function() {
  var uhigh = window[user.pokemon].highest_attack
  var ulow = window[user.pokemon].lowest_attack
  return Math.round(Math.random().toFixed(1)*uhigh)
}        

var comp_attack = function() {
  var chigh = window[comp.pokemon].highest_attack
  var clow = window[comp.pokemon].lowest_attack
  return Math.round(Math.random().toFixed(1)*chigh)
}

These two functions do exactly the same exact thing, except they are for two very specific instances of a class. This is also terrible practice, as nothing is flexible here and functions not only repeat themselves but rely on two very specific objects to be, again, in the global scope.

Instead, these two functions should become one instance method of the player class:

player.prototype.attack = function() {
  var uhigh = this.pokemon.highest_attack
  var ulow = this.pokemon.lowest_attack
  return Math.round(Math.random().toFixed(1)*uhigh)
}

Then, you should use this methods instead of those other two functions.

user.attack();
comp.attack();

You may want to rename to doAttack as the wording can be confusing.

---

function player(type, name, pokemon) {
    this.type = type;
    this.name = name;
    this.pokemon = pokemon;
}

var user = new player("User", "Arhum", "Charizard");

Are there any types other than "User" and "Computer"? If not, then you should turn this property into a boolean value something like this:

function player(isComputer, name, pokemon) {
    this.isComputer = isComputer;
    this.name = name;
    this.pokemon = pokemon;
}

var user = new player(false, "Arhum", "Charizard");

---

var Charizard = new pokemon("Charizard", 90, 30, 1, 15);
var Blastoise = new pokemon("Blastoise", 95, 28, 1, 14);
var Venasaur = new pokemon("Venasaur", 100, 26, 1, 13);

var poke_choice = new Array ("Charizard", "Blastoise", "Venasaur")

Now that we're done relying on these three pokemon variables to be in the global scope, we can clean up this part of the code, too.

We don't need to explicitly store the three pokemon variables anymore with their variable names; those can just go straight into the array. However, when we do that, we can no longer access the pokemon by name so it would be best to create a map/object for this situation:

var pokeChoice = {
    Charizard: new pokemon(...),
    Blastoise: new pokemon(...),
    ...
}

With this in place, this

for(i = 0; i<= 2; i++) {
  if (poke_choice[i] === user.pokemon) {
    poke_choice.splice(i,1);
  }
}

can be simplified to:

if(pokeChoice.hasOwnProperty(user.pokemon.name)) {
    delete pokeChoice[user.pokemon.name];
}

---

Here is your full code now:

function pokemon(name, hp, critical_attack, lowest_attack, highest_attack) {
    this.name = name;
    this.hp = hp;
    this.critical_attack = critical_attack;
    this.lowest_attack = lowest_attack;
    this.highest_attack = highest_attack;   
}
var pokeChoice = {
    Charizard: new pokemon("Charizard", 90, 30, 1, 15),
    Blastoise: new pokemon("Blastoise", 95, 28, 1, 14),
    Venasaur: new pokemon("Venasaur", 100, 26, 1, 13)
};

function player(isComputer, name, pokemon) {
    this.isComputer = isComputer;
    this.name = name;
    this.pokemon = pokemon;
}
player.prototype.attack = function() {
    var uhigh = this.pokemon.highest_attack
    var ulow = this.pokemon.lowest_attack
    return Math.round(Math.random().toFixed(1)*uhigh)
}

var user = new player("User", "Arhum", pokeChoice.Charizard);
if(pokeChoice.hasOwnProperty(user.pokemon.name)) {
    delete pokeChoice[user.pokemon.name];
}

var comp_choice = function() {
    var pokeKeys = Object.keys(pokeChoice);
    var pokeAmt = pokeKeys.length;
    return pokeChoice[pokeKeys[Math.round(Math.random() * (pokeAmt-1))]]
}
var comp = new player("Computer","Red", comp_choice());    

console.log(user.attack())
console.log(comp.attack())

I made some other small changes here and there. If you have any questions, feel free to ask.

Also, the naming convection in JavaScript is:

• PascalCase for classes.
• camelCase for functions.

You are breaking these rules in some cases.

Answer 2

In addition to @SirPython's answer:

• Strict mode. Put 'use strict' (or 'use strict';) at the top to make your file run in strict mode. This basically throws more errors, prohibits things like with (obj) { }, and changes the behaviour of things like eval(). It can be a lifesaver when it comes to finding things like mistyped variables.

• IIFEs. If you're running JavaScript in a browser, it's a good idea to wrap it in an Immediately-Invoked Function Expression like this, preventing variables from being created as properties of window:

  ;(function () {
    // All your code
  })()
  

• Rename your variables from things like comp_choice to compChoice (or computerChoice). JavaScript uses CamelCase instead of snake_case.

• Rename your player and pokemon functions to Player and Pokemon. There's a convention that constructors (things you call with new) begin with a capital letter. Likewise, Charizard, Blastoise, and Venasaur should be renamed to charizard, blastoise, and venasaur as they're not constructors.
• Avoid new Array('foo', 'bar', 'baz'). Use ['foo', 'bar', 'baz'] instead. new Array() is confusing, as if you try to call it with a number like new Array(3), it'll do [undefined, undefined, undefined] instead of [3]. It's a good idea to get into the practice of using [] instead; it's shorter anyway.
• Don't do for (i = 0; i < 5; i++) { }. This creates a global variable called i, which can cause bugs if there are other variables called that. Do var i; for (i = 0; i < 5; i++) { }, as this makes it a local variable.
• Inside your user_attack and comp_attack functions, change the variables from uhigh, ulow, chigh, and clow to just high, low, high, and low. Scoping means that the names don't collide.
• Semicolons. Use them. Or don't. If you want to use them, everything except things like (for instance) if (foo) { }, while (foo) { }, for (...) { }, and function foo () { } should end with ;. If you don't want to use them, make sure you understand the rules.

• While functions can technically have up to 255 parameters, having too many can make them difficult to use. For example:

  function lotsOfParametersHere (a, b, c, d, e, f, g) {
    // Do something
  }
  lotsOfParametersHere(4, 7, 97, 123, 76, 23, 42)
  
  // Can be replaced with:
  
  function easierToUnderstand (options) {
    var a = options.a
    var b = options.b
    var c = options.c
    var d = options.d
    var e = options.e
    var f = options.f
    var g = options.g
  
    // Do something
  }
  easierToUnderstand({ a: 4, b: 7, c: 97, d: 123, e: 76, f: 23, g: 42 })
  
  // Or (ES6):
  
  function thisUsesES6Destructuring ({ a, b, c, d, e, f, g }) {
    // Do something
  }