1
\$\begingroup\$

So, this is for a clicker game I'm trying to build with Pokémon data drawn from the PokéAPI. Since I'm using Vue.js for the project, Vuex is used to store and share the data between components and I'm persisting and loading the state with LocalForage to IndexedDb.

When constructing a Pokemon-object from the Pokemon-class I'm having two different ways I get the data to construct the object.
From the API I'll get the data structured one way and from the IndexedDb I get the 1to1 JSON-stringified version of the Pokemon-object. So just the raw JSON without the class-methods or any properties tied to other custom classes or any enums. You probably already knew that, but I didn't.
(At first I was just using an init-method to set the async properties, but that changed when I started using IndexedDb)

That's why I got the basic constructor method to build the object anew from the saved object in the IndexedDb and a constuctFromApiResponse method to map the pokeapi response to my Pokemon-object.
I'm trying to figure out what's the best way to write as little code as possible to stay DRY. (while keeping any type-inferences and other goodies that PHPStorm is able to make without much usage of JSHint (although I'm fine with adding hints, just didn't add them here everywhere because I'm going back and forth between the different constructor-approaches))

Pokemon.js

import Item from './Item'
import { Pokedex } from 'pokeapi-js-wrapper'
import Type from './Type'

const P = new Pokedex()

/**
 * @class Pokemon
 */
class Pokemon {
  constructor ({
    dexNum, name, level, types, baseStats, stats, sprites, hp, exp, item,
    growthRate = '', catchRate = 0, evolutions = {}
  }) {
    Object.assign(this, {
      dexNum,
      name,
      level,
      baseStats,
      stats,
      hp,
      exp,
      sprites,
      growthRate,
      catchRate,
      evolutions
    })

    /**
     * @type {Type[]}
     */
    this.types = types.map((type) => Type[type.name.toUpperCase()])

    /**
     * @type {Move[]}
     */
    this.moves = []

    /**
     * @type {Item}
     */
    this.item = item ? (item instanceof Item ? item : new Item(item)) : null
  }

  /* eslint-disable-next-line camelcase */
  static async constructFromApiResponse ({ id, name, types, stats, base_experience, sprites }, level) {
    const self = {
      dexNum: id,
      name,
      level,
      sprites,

      hp: 0,
      exp: 0,
      item: null
    }

    self.types = types.map((type) => Type[type.type.name.toUpperCase()])

    self.baseStats = {}
    for (const stat of stats) {
      self.baseStats[stat.stat.name] = stat.base_stat
    }
    /* eslint-disable-next-line camelcase */
    self.baseStats.exp = base_experience
    self.stats = self.baseStats

    self.hp = self.stats.hp * level
    const p = new Pokemon(self)

    p.calculateStats()
    await p.setGrowthRate()
    await p.setCatchRate()
    await p.setEvolutionChain()

    return p
  }

  async setGrowthRate () {
    this.growthRate = await this.getSpecies()
      .then((response) => response.growth_rate.name)
  }

  async setCatchRate () {
    this.catchRate = await this.getSpecies()
      .then((response) => response.capture_rate)
  }

  async setEvolutionChain () {
    const evolvesTo = (next, prevDexNum) => {
      for (const ev of next) {
        const details = ev.evolution_details[0]
        const evolution = {
          level: details.min_level,
          method: details.trigger.name,
          item: details.item || details.held_item,
          dexnum: ev.species.url.split('/').slice(-2, -1)[0]
        }

        if (!this.evolutions.hasOwnProperty(prevDexNum)) {
          this.evolutions[prevDexNum] = []
        }
        this.evolutions[prevDexNum].push(evolution)

        // recurse
        if (ev.hasOwnProperty('evolves_to') &&
            ev.evolves_to.length > 0) {
          evolvesTo(ev.evolves_to, evolution.dexnum)
        }
      }
    }

    const evoChainUrl = await this.getSpecies()
      .then((response) => response.evolution_chain.url)

    await P.resource(evoChainUrl)
      .then((response) => evolvesTo(
        response.chain.evolves_to,
        response.chain.species.url.split('/').slice(-2, -1)[0])
      )
  }

  /**
   * Get species entry
   *
   * @returns {Promise}
   */
  getSpecies () {
    return P.resource('/api/v2/pokemon-species/' + this.dexNum)
  }

  /**
   * @param toDexNum {number}
   *
   * @returns {Promise<void>}
   */
  async evolve (toDexNum = 0) {
    const evolutions = this.evolutions[this.dexNum]
    let evolution = evolutions[0]

    // get target evolution if there's more than one
    if (toDexNum > 0) {
      for (const ev of evolutions) {
        if (this.evolutions.dexNum === toDexNum) {
          evolution = ev
          break
        }
      }
    }

    if (evolution.method === 'item' ||
        (evolution.method === 'trade' && evolution.item !== null)) {
      // TODO: remove held item
    }

    const evolved = await Pokemon.constructFromApiResponse(
      await P.resource('/api/v2/pokemon/' + evolution.dexnum),
      this.level
    )

    // re-set class properties
    this.name = evolved.name
    this.dexNum = evolved.dexNum
    this.types = evolved.types
    this.baseStats = evolved.baseStats
    this.stats = evolved.stats
    this.sprites = evolved.sprites
    this.growthRate = evolved.growthRate
    this.catchRate = evolved.catchRate
  }

  // [...] skipping review-unrelated methods

}

export default Pokemon

I was doing this.name = name before, but was looking for other ways to set the properties without repeating myself that much. But I don't know if the Object.assign method is really considered "cleaner" or better to read.
Also depending on the constructor-approach, maybe there's a better way to re-set the properties for the evolution in the evolve-method.
In addition I don't know what to do about the many function parameters. eslint with standardjs doesn't seem to have an opinion about this.

Item.js


/**
 * @class Item
 */
class Item {
  constructor ({ id, name, cost, attributes, category, sprites, quantity, effect, triggerOnHold, onUse }) {
    Object.assign(this, {
      id,
      name,
      cost,
      attributes,
      category,
      sprites,
      quantity,
      effect,
      triggerOnHold,
      onUse
    })
  }

  static async constructFromApiResponse ({ id, name, cost, attributes, category, sprites }, quantity, effect, triggerOnHold, onUse) {
    const self = {
      id,
      name,
      cost,
      attributes,
      category: category.name,
      sprites,
      quantity,
      effect,
      triggerOnHold,
      onUse
    }

    const item = new Item(self)

    return item
  }

  // [...] skipping review-unrelated methods
}

export default Item

Same thing with Item, although I don't have properties here that need any special treatment just yet. That's when I thought I'd have another look at TypeScript and saw that you can just omit the constructor function body when you're just doing assignments like this.name = name. Maybe I'll rewrite it in TypeScript then if that leads me to DRY-haven. But I'm not sure about that yet.


The following are the two files where I'm using the two different Pokemon-constructor methods, for reference.

store.js

import Item from './classes/Item'
import LocalForage from 'localforage'
import Pokemon from './classes/Pokemon'
import Vue from 'vue'
import Vuex from 'vuex'

Vue.use(Vuex)

const lf = LocalForage.createInstance({
  driver: LocalForage.INDEXEDDB,
  name: 'PokeClicker',
  version: 1.0,
  storeName: 'pokeclicker'
})

const persistPlugin = store => {
  store.subscribe((mutations, state) => {
    for (const key in state) {
      if (!state.hasOwnProperty(key)) {
        continue
      }

      const value = state[key]
      lf.setItem(key, value)
    }
  })
}

const store = new Vuex.Store({
  plugins: [
    persistPlugin
  ],

  state: {
    bags: {
      misc: [],
      medicine: [],
      pokeballs: [],
      // machines: [],
      berries: [],
      // mail: [],
      battle: [],
      key: []
    },
    team: [],
    box: [],
    money: 0,
    reputation: 0,
    route: {},
    opponent: {},
    enemy: {}
  },

  mutations: {
    async init (state, data) {
      // init bag
      for (const key in data.bags) {
        if (!data.bags.hasOwnProperty(key)) {
          continue
        }

        const bag = data.bags[key]
        for (const idx in bag) {
          const item = bag[idx]
          data.bags[bag][key] = new Item(item)
        }
      }

      // init team
      for (const idx in data.team) {
        const pkmn = data.team[idx]
        if (!(pkmn instanceof Pokemon)) {
          data.team[idx] = new Pokemon(pkmn)
        }
      }

      Object.assign(state, data)
    },

    // [...] skipping review-unrelated methods
  },

  actions: {
    async init ({ commit }) {
      let state = {}
      await lf.iterate((value, key) => {
        state[key] = value
      })
      await commit('init', state)
    }
  }
})

export default store

In the store's init action/mutation I re-set the raw JSON data from IndexedDb to the corresponding classes so I can use their methods properly after loading them.

Explore.vue

<template>
    <div class="team">
      <!-- Team -->
      <team-pkmn
        v-for="pokemon in $store.state.team"
        :key="pokemon.id"
        :pokemon="pokemon"
      />
    </div>

    <!-- [...] skipping review-unrelated code -->
  </div>
</template>

<script>
import { Pokedex } from 'pokeapi-js-wrapper'
import Pokemon from '../../classes/Pokemon'
import TeamPkmn from '../Pokemon/TeamPkmn'

const P = new Pokedex()

export default {
  components: {
    TeamPkmn
  },

  data () {
    return {}
  },

  async created () {
    await this.$store.dispatch('init')

    if (!this.$store.state.team.length) {
      const ids = [679, 10, 13, 46, 48, 165, 167, 333, 290, 557, 736, 595, 742, 751, 349, 220, 366]
      for (const id of ids.reverse()) {
        const p = await Pokemon.constructFromApiResponse(
          await P.resource('/api/v2/pokemon/' + id), 5
        )
        this.$store.state.team.push(p)
      }
    }
  }
}
</script>

In the component's async created method I load some Pokémon from the Api, and create the Pokemon-object with the constructFromApiResponse-method, if none are in the store, so not loaded from the client's Db.

\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

One thing that stood out for me is that you’re doing multiple HTTP calls to the same end-point during the constructFromApiResponse when you could just do one, i.e.

this.getSpecies().then(resp => new Pokemon({
  growthRate: resp.growth_rate.name
  , catchRate: resp.capture_rate.name
  , baseStats: resp.stats.reduce((acc, {base_stat, stat: {name}}) => {
    acc[name] = base_stat
    return acc
  }, {exp: resp.base_experience}) // just an example of property conversion
  , // etc.
})

One should always strive for minimizing the number of HTTP calls.

Also I would consider using the same constructor for both, from HTTP and local, and doing the property conversions for HTTP in there.

Speaking of properties, there are other options in case you’re interested. One is a method:

// inside class definition
hp() { this.baseStats.hp * this.level }

and another is a getter:

get hp() { this.baseStats.hp * this.level }

where the former needs to be executed (poke.hp()), while the latter acts more like a computed property (poke.hp). Though neither wouldn’t get serialized into JSON.

\$\endgroup\$
2
\$\begingroup\$

To answer one part of your question, I think you can assign all the properties at once with Object.assign:

class Pokemon {

    constructor(data) {
        Object.assign(this, data);
    }

    getStats() {
        return this.species + ' ' + this.age;
    }
}

const json = '{"species":"pikachu", "age":12}'
const obj = JSON.parse(json);

let u = new Pokemon(obj)
console.log(u.getStats()). // will log pikachu 12
\$\endgroup\$
2
\$\begingroup\$

I see a couple places where there is a for...in loop and then the first line within the block checks to see if the key is not a property - e.g.

for (const key in state) {
  if (!state.hasOwnProperty(key)) {
    continue
  }

and

for (const key in data.bags) {
  if (!data.bags.hasOwnProperty(key)) {
    continue
  }

Did you consider using Object.keys() to get the array of keys and iterating over those? Then perhaps the calls to .hasOwnProperty() could be eliminated.


In the store actions method init, state is never re-assigned:

actions: {
  async init ({ commit }) {
    let state = {}
    await lf.iterate((value, key) => {
      state[key] = value
    })
    await commit('init', state)

You could use const instead of let to declare state.


I see two nearly identical lines:

/**
 * @type {Type[]}
 */
this.types = types.map((type) => Type[type.name.toUpperCase()])

and shortly after that:

self.types = types.map((type) => Type[type.type.name.toUpperCase()])

if that mapping happens frequently in your code, perhaps it would be wise to abstract that arrow function into a named function that can be referenced wherever needed.


In the callback function subscribed on the store (in persistPlugin) the variable value is only used once after it is assigned:

const value = state[key]
  lf.setItem(key, value)

That could be simplified to a single line:

lf.setItem(key, state[key])

This would require less memory and one less line.

\$\endgroup\$

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