tmedwards / sugarcube-2

SugarCube is a free (gratis and libre) story format for Twine/Twee.
https://www.motoslave.net/sugarcube/2/
BSD 2-Clause "Simplified" License
185 stars 42 forks source link

Unused code in develop/State.prngCreate #282

Closed BawdyInkSlinger closed 1 year ago

BawdyInkSlinger commented 1 year ago

Describe the bug. I believe there is code in State.prngCreate that isn't used. I'm not sure if this is a bug or just some code that should be removed:

    /*
        Returns a new PRNG wrapper object.
    */
    function prngCreate(seed, mixEntropy) {
        const newPrng = new Math.seedrandom(seed, { // eslint-disable-line new-cap
            entropy : Boolean(mixEntropy),
            pass    : (prng, seed) => ({ prng, seed })
        });

        return Object.create(null, {
            prng : {
                value : newPrng.prng
            },
            seed : {
                value : newPrng.seed
            },
            pull : {
                writable : true,
                value    : 0
            },
            random() { // this is not a property signature
                ++this.pull;
                return this.prng();
            }
        });
    }

I don't think random() does anything as written, because it doesn't have the type of a property.

tmedwards commented 1 year ago

I'm unsure if you're having a verbiage or a knowledge breakdown, but while that is broken, it's not what you claimed.

It is an object literal method shorthand notation, so it's definitely a property. The issue is that it should have been a property descriptor instead. E.g.,

random : {
    value() {
        ++this.pull;
        return this.prng();
    }
}

Regardless. It was most certainly broken, though that wasn't the only issue with that rewritten code. Both errors were caught because of you, so thanks.

BawdyInkSlinger commented 1 year ago

I'm unsure if you're having a verbiage or a knowledge breakdown, but while that is broken, it's not what you claimed.

why-not-both-why-not

It is an object literal method shorthand notation, so it's definitely a property. The issue is that it should have been a property descriptor instead.

Yep, sorry, that's what I should have said.

Regardless. It was most certainly broken, though that wasn't the only issue with that rewritten code. Both errors were caught because of you, so thanks.

Happy to help! Should I close this issue?

tmedwards commented 1 year ago

You can if you want to, but there's no need. I referenced the issue in the commit messages, so it will automatically be closed when develop is merged into master.