CODEWITHSUNDEEP
javascript
Fast Arrows
Fast Arrows

Reputation: 329

My code for some reason shows NaN instead of number

Why am I getting NaN instead of number when I click the button #plr1button1? Like I don't understand it I am looking at it for like 2 hours and I still can't figure out the mistake.

class Fighter {
    constructor(atk, def, hp) {
        this.atk = atk;
        this.def = def;
        this.hp = hp;
    }
}
var Fighter1 = new Fighter(40, 5, 100);
var Fighter2 = new Fighter(30, 20, 100);
Fighter1.attack = function() {
    var attack1 = this.atk + (Math.floor(Math.random() * 5) - 5) - Fighter2.def;
    Fighter2.hp = Fighter2.hp - attack1;
    document.getElementById("hp2").innerHTML = Fighter2.hp;
}
Fighter2.attack = function() {
    var attack1 = this.atk + (Math.floor(Math.random() * 5) - 5) - Fighter1.def;
    Fighter1.hp = Fighter1.hp - attack1;
    document.getElementById("hp1").innerHTML = Fighter1.hp;
}
function random() {
    var randomNum =  Math.floor(Math.random() * 6) + 1;
    /*var randomNum2 =  Math.floor(Math.random() * 6) + 1;
    var randomNum3 =  Math.floor(Math.random() * 6) + 1;*/
    if (randomNum === 1) {
        document.getElementById("plr1button1").innerHTML = "Attack";
        $("#plr1button1").bind("click", Fighter1.attack);
        document.getElementById("plr2button1").innerHTML = "Attack";
    }
}

Upvotes: 0

Views: 90

Answers (2)

raina77ow
raina77ow

Reputation: 106375

You lose the context with that event handler; use .bind to fix it.

$("#plr1button1").bind("click", Fighter1.attack.bind(Fighter1));

It's easy to detect it: just put the breakpoint into attack method and check what's this equal to when the method is called by as click event handler.

There are several other issues with your code.

First, you can easily abstract a function that generates a random integer number in the given range - and reuse it instead of repeating the whole Math.floor(Math.random... snippet.

Second, you mixed two concerns in your attack code, changing both state (of attacked Fighter) and its representation. It's usually better to separate those.

Finally, you unnecessarily hardcoded all the actions in two Fighter instance - instead of using prototype to store a single function.

For example, there's one way how this can be simplified:

class Fighter {
    constructor(atk, def, hp) {
        this.atk = atk;
        this.def = def;
        this.hp = hp;
    }

    attack(enemy) {
        const hits = this.atk + _randomInRange(0, 5) - enemy.def;
        enemy.hp -= hits;
        enemy.render();
    }

    render() {
        // updates the representation of Fighter
    }
}

function _randomInRange(from, to) {
  return from + Math.floor( Math.random() * (to + 1 - from) );
}

And here's a small (and incomplete) demo of that approach in action.

Upvotes: 3

samanime
samanime

Reputation: 26527

It's because how you are calling it, this is not Fighter1, it's the element that triggered the event (since you're using jQuery; vanilla JS it would be the event object) inside of Fighter1.attack.

When you call a function directly like you are, it's going to use the function but won't maintain it's usual context. I.e., Fighter1.attack in a callback is not the same as Fighter1.attack().

What you need to do is bind the functions to their respective this so they maintain that inside.

Change your callback like this:

$("#plr1button1").bind("click", Fighter1.attack.bind(Fighter1));

Then, when it's triggered, this will be Fighter1 as you intended.

You could also bind the function overall to not have to rebind:

Fighter1.attack = (function() {
    var attack1 = this.atk + (Math.floor(Math.random() * 5) - 5) - Fighter2.def;
    Fighter2.hp = Fighter2.hp - attack1;
    document.getElementById("hp2").innerHTML = Fighter2.hp;
}).bind(Fighter1);

Repeat for Fighter2 as well.

Upvotes: 1

Related Questions