0

Description

I am writing a Discord Bot in NodeJS and am currently experiencing a very odd issue.

What I am wanting to do is to get the result of health via method getHP(), afterwards updating the health property with the setHP() method.

This works for one class, but not for another. So basically, the code is practically the same, but for the other class it does not update the property.

I am calling both the classes their setHP() methods in their constructors.

Code:


// Player.js - This works and displays: { current: 98, max: 98 }

class Player {
  constructor(member, msg) {
    this.member = member
    this.msg = msg
    this.setHP()
  }

  health = {}
  setHP() {
    this.getHP.then(hp => {
      this.health = { current: hp.current, max: hp.current }
    })
  }

  get getHP() {
    return new Promise(async (resolve) => {

      const stats = await this.stats
      resolve(stats.find(stat => stat.id === 'health'))
    })
  } 
  
  get stats() {
    return new Promise(async (resolve) => {
      const result = await DB.query(`select stats from members where member_id = ${this.member.id} `)
      resolve(JSON.parse(result[0][0].stats))
    })
  }

  get difficulty() {
    return new Promise(async (resolve) => {
      const result = await DB.query(`select difficulty from members where member_id = ${this.member.id} `)
      resolve(result[0][0].difficulty)
    })
  }
}

// Enemy.js - Doesn't work and displays: {}

class Enemy {
  constructor(player) {
    this.player = player
    this.setHP()
  }

  hp = {}
  setHP() {
    this.getHP.then(int => {
      this.hp = { current: int, max: int }
    })
  }

  get getHP() {
    return new Promise(async (resolve) => {
      const difficulty = await this.player.difficulty
      const int = Math.floor(this.player.health.current * (difficulty * (Math.random() * 0.10 + 0.95)))
      resolve(int)
    })
  }

// minion_fight.js - Where the classes are used

const Enemy = require("Enemy.js")
const Player = require("Player.js")

module.exports.execute = async (msg) => {
  const player = new Player(msg.member, msg)
  const enemy = new Enemy(player)

  // ...
}
Sprity
  • 3
  • 2
  • 2
    Can you complete the code so that we can see what all those referenced properties are, like `this.stats`, `this.player.difficulty`, ...etc? BTW, it is an antipattern to use `new Promise` when you already have a promise to work with. – trincot Feb 26 '22 at 16:39
  • @trincot updated. Is this any better? – Sprity Feb 26 '22 at 17:00
  • But `difficulty` is a sibling method of `getHP`, so it is not right to access it as `this.player.difficulty`. You leave us guessing how your code is organised. You should present it in **one** code block, including the `class` heading line, so that it represents valid JavaScript, and it is no more ambiguous which method belongs to what class. – trincot Feb 26 '22 at 17:04
  • @trincot updated. Hope this is better! – Sprity Feb 26 '22 at 17:18
  • There is still something missing. Where is the player object constructed that is passed to the `Enemy` constructor? Can you show the code that creates the player instance and the enemy instance? NB: I see what is wrong, but I need to be sure about this aspect first. – trincot Feb 26 '22 at 17:22
  • 1
    @trincot I hope you meant this! Updated code is below where the instances are called. – Sprity Feb 26 '22 at 17:27
  • [Never pass an `async function` as the executor to `new Promise`](https://stackoverflow.com/q/43036229/1048572)! Also avoid getters that result in promises, use a normal `getStats()`/`getHP()`/`getDifficulty()` method instead to avoid confusion. And finally, [don't call asynchronous methods from a constructor](https://stackoverflow.com/q/24398699/1048572). You never know when (or whether) your `.health`/`.hp` property will have the right value. – Bergi Feb 26 '22 at 17:37
  • @Bergi Where am I passing an async function as the executor to a promise? Also, I think it's ok calling the async method from within the constructor in this case because it isn't being updated in any other way. Correct me if I'm wrong – Sprity Feb 26 '22 at 17:47
  • @Sprity In `return new Promise(async (resolve) => {`. And the problem with calling `setHP()` from the constructor is not that `.hp` might be updated elsewhere, but that you don't know when it will be updated asynchronously. You haven't shown where you actually access the property, so I can't point out an example. – Bergi Feb 26 '22 at 18:03

1 Answers1

0

The main issue is that the player instance has a pending promise that will eventually resolve and set the player's health property. But before that happens, the enemy instance is created, and it accesses the above mentioned health property from the given player before it has been set. So this.player.health.current cannot be evaluated.

It is better to:

  • Avoid launching asynchronous tasks in a constructor. Instead create methods that do this.
  • Avoid creating promises with new Promise, when there is already a promise to await. This is an anti-pattern.
  • Don't use getters/setters for asynchronous tasks. Just make them asynchronous methods -- it will make the code easier to understand.
  • Please terminate your statements with a semi-colon. You don't really want to make the interpretation of your code dependent on the automatic semi-colon insertion algorithm.

Here is the suggested correction -- but I didn't test it, so I hope you'll at least get the gist of the proposed changes:

// Player.js

class Player {
  constructor(member, msg) {
    this.member = member;
    this.msg = msg;
  }

  health = {}

  async setHP() {
    const hp = await this.getHP();
    this.health = { current: hp.current, max: hp.current };
    return this.health;
  }

  async getHP() {
    const stats = await this.stats();
    return stats.find(stat => stat.id === 'health');
  } 
  
  async stats() {
    const result = await DB.query(`select stats from members where member_id = ${this.member.id} `);
    return JSON.parse(result[0][0].stats);
  }

  async difficulty() {
    const result = await DB.query(`select difficulty from members where member_id = ${this.member.id} `);
    return result[0][0].difficulty;
  }
}

// Enemy.js

class Enemy {
  constructor(player) {
    this.player = player;
  }

  hp = {}
  
  async setHP() {
    const current = await this.getHP();
    this.hp = { current, max: int };
    return this.hp;
  }

  async getHP() {
    const playerHealth = await this.player.getHP(); // To be sure the promise is resolved!
    const difficulty = await this.player.difficulty();
    return Math.floor(playerHealth.current * (difficulty * (Math.random() * 0.10 + 0.95)));
  }
}

// minion_fight.js

const Enemy = require("Enemy.js")
const Player = require("Player.js")

module.exports.execute = async (msg) => {
  const player = new Player(msg.member, msg);
  const enemy = new Enemy(player);
  await enemy.setHP();
  // ...
}
trincot
  • 317,000
  • 35
  • 244
  • 286