Weapon class firing logic in JavaScript

Question

I'm making a program to compare Borderlands 3 weapon damage over time in a graphic. There's still some work to be done, like implementing elemental damage, but I'd like to know if I'm doing a good job for now regarding the main firing logic.

My biggest concerns are:

• General readability.

• WeaponReload and WeaponFire extend WeaponAction, a class containing the properties .startTime, .endTime and .duration (included after the Weapon class code) and both are being treated as if they're the same. I think some other languages can do something similar using structs and protocols to check conformity and I'm not sure if this is the right way to do it in JavaScript.

• I don't know whether I'm doing a good use of the try {...} catch {...} statement in Weapon.fullAuto or if this logic could be improved upon using other methods.

class Weapon {
    constructor (damage, fireRate, reloadSpeed, magazineSize, elementalDamage, elementalChance) {
        this.damage = damage
        this.fireRate = fireRate
        this.reloadSpeed = reloadSpeed
        this.magazineSize = magazineSize
        this.elementalDamage = elementalDamage
        this.elementalChance = elementalChance

        this.bulletsLeftInMagazine = magazineSize
    }
    get timePerShot () {
        return 1 / this.fireRate
    }
    fire (startTime = 0) {
        if (this.bulletsLeftInMagazine < 1)
            throw new Error("Not enough bullets in the magazine")

        let whenCanFireAgain = startTime + this.timePerShot

        --this.bulletsLeftInMagazine

        return new WeaponShot(startTime, whenCanFireAgain, this.damage)
    }
    reload (startTime = 0) {
        let whenCanFireAgain = startTime + this.reloadSpeed

        this.bulletsLeftInMagazine = this.magazineSize

        return new WeaponReload(startTime, whenCanFireAgain)
    }
    fullAuto (duration, startTime = 0) {
        let localActionStartTime = 0,
            globalActionStartTime = startTime,
            actions = []

        while (duration > localActionStartTime) {

            let localActionEndTime,
                globalActionEndTime,
                action

            try {
                action = this.fire(globalActionStartTime)
            } catch {
                action = this.reload(globalActionStartTime)
            }

            actions.push(action)

            localActionStartTime += action.duration
            globalActionStartTime += action.duration

        }

        return actions
    }
}

class WeaponAction {
    constructor (startTime, endTime, damageDealt) {
        this.startTime = startTime
        this.endTime = endTime
        this.damageDealt = damageDealt
    }
    get duration () {
        return this.endTime - this.startTime
    }
}

Answer 1

I understand semicolons in JavaScript are optional, however I believe you should always use them. It's easy to have something break later on because you neglected a semi-colon and it lost the whitespace which saved it before in a compression/generation/eval parse.

You could declare the timePerShot as a variable, to ensure the amount is not being calculated on every call (Although I'm pretty sure the parser or cache already solves this).

Use exceptions / errors in exceptional cases only. An example being making an HTTP call; The target could not exist, the internet could be down, or other cases that are out of your control.

Catching exceptions is also going to be very slow. You can return a value such as false instead. And/or you could do the check outside of the function.

Answer 2

There is not much I would add to dustytrash's answer:

• prefer const over let when a variable does not change
• use blank lines only for creating logical regions; to me, an assignment to an instance variable is not a logical region

reload (startTime = 0) {
    const whenCanFireAgain = startTime + this.reloadSpeed;
    this.bulletsLeftInMagazine = this.magazineSize;
    return new WeaponReload(startTime, whenCanFireAgain);
}

instead of..

reload (startTime = 0) {
    let whenCanFireAgain = startTime + this.reloadSpeed

    this.bulletsLeftInMagazine = this.magazineSize

    return new WeaponReload(startTime, whenCanFireAgain)
}

Answer 3

The getter method Weapon.timePerShot will return 1 / this.fireRate. There doesn't appear to be anything to prevent fireRate from being 0. If that is the case, Weapon.timePerShot would return Infinity, which would lead to whenCanFireAgain also being set to Infinity. In some programming languages dividing by zero would lead to an exception being thrown or a warning being emitted. It is wise to avoid this scenario.

---

The while loop in Weapon.fullAuto() declares three variables:

let localActionEndTime,
    globalActionEndTime,
    action

The first two variables don't appear to be used. Unless they are to be used by future code, they can be removed.