8
\$\begingroup\$

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
    }
}
\$\endgroup\$
  • \$\begingroup\$ @dfhwze I rolled back your edit because the animation tag does not match this question contents. The project involves a graphic representing the damage over time of the weapon, but this class does not manage animation at all. \$\endgroup\$ – Tiago Marinho Sep 17 '19 at 11:08
  • \$\begingroup\$ Could you include the code for those other 2 classes? \$\endgroup\$ – dfhwze Sep 17 '19 at 11:08
  • \$\begingroup\$ No problem rolling back that tag. To me this is an animation, just not visible yet on a UI. \$\endgroup\$ – dfhwze Sep 17 '19 at 11:10
  • 2
    \$\begingroup\$ @dfhwze I included the relevant code for the additional classes. Since they're both just extensions of WeaponAction I only added that single class code. The other two are the same thing, except for .damageDealt but that does nothing. \$\endgroup\$ – Tiago Marinho Sep 17 '19 at 11:13
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ And, if you don't like typing semicolons for each line, just use VS Code with prettier and auto correct on save with ESLint. \$\endgroup\$ – AleksandrH Sep 17 '19 at 18:24
  • 2
    \$\begingroup\$ The timePerShot function is not really a magic number since it's just getting the reciprocal of fireRate. Separating out a variable for this would be like separating out a variable for the power of 2 in a squaring function. \$\endgroup\$ – Shadetheartist Sep 18 '19 at 0:40
  • \$\begingroup\$ I'm not sure if it's the same with JS, but at least for python, a try-except that is expected to succeed most of the time is faster on average than an if-else. \$\endgroup\$ – Mars Sep 18 '19 at 1:55
  • \$\begingroup\$ Ah... ran a short test. For a case where it failed 1/30 times, if was faster in python by about 10%. For a case when it failed 1/300 times, try was faster by about 40%. Most guns don't have 300 bullets though, so I'd guess if-else is best here \$\endgroup\$ – Mars Sep 18 '19 at 2:03
  • \$\begingroup\$ secondsPerSecond = 1 is the weirdest code I've ever written, but it does make sense though (we could be measuring in millisseconds then that would be millissecondsPerSecond = 1000 instead). Also, the input on using a false return value instead of catching exceptions is really helpful. timePerShot is a getter/setter because fireRate could be dynamic, but since it's not implemented in current code I see your point. Thanks for your review! \$\endgroup\$ – Tiago Marinho Sep 18 '19 at 11:12
5
\$\begingroup\$

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)
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I always forget const exists, and the reload function does indeed have unnecessary blank lines. Thanks! \$\endgroup\$ – Tiago Marinho Sep 18 '19 at 11:16
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ A time per shot of Infinity seems like a cool feature for a game, though. :p \$\endgroup\$ – dfhwze Sep 18 '19 at 4:08
  • \$\begingroup\$ Well, it's expected whenCanFireAgain being infinite if the weapon has 0 fire rate. I think nothing actually breaks because of it (not even Weapon.fullAuto) but I'm yet to test it. Good catch on that dead code, and thanks for your review! \$\endgroup\$ – Tiago Marinho Sep 18 '19 at 11:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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