Decoder.finish() incorrect?

[joedrago]

I believe your decoder's finish() function is wrong, but I don't know enough of the specifics of the function to pin down exactly why. Either way, you're referencing a variable (bits) that only exists in an Encoder, so there is at least one issue there. Just figured I'd mention it.

[agnoster]

I think you're right! I haven't worked on this in a while... hmm. A test case would be nice. It's also possible that it's wrong without resulting in wrong behavior, which is one of those "tree falling in the forest and no one around" koan thingies...

[agnoster]

Generally speaking, this should be rewritten as a TransformStream. It's OOOOLD.

[joedrago]

I could go see if I could find the original stream that got me to look at the code, but the idea was something like step was 4 and byte was 32 going into finish(), and instead of making the last character a 2, it just made it a 0. I'm not sure if that is helpful or not.

[WebReflection]

Hello there.

The Decoder augment the instance with a this.finish = method that indeed has undefined bits variable in there: https://github.com/agnoster/base32-js/blob/master/lib/base32.js#L141

I believe the reason is that this.finish is a copy and paste from the Encoder constructor.

However, accordingly with these lines: https://github.com/agnoster/base32-js/blob/master/lib/base32.js#L129-L136

        skip += 5
        if (skip >= 8) {
            // we have enough to preduce output
            this.output += String.fromCharCode(byte)
            skip -= 8
            if (skip > 0) byte = (val << (5 - skip)) & 255
            else byte = 0
        }

it's clear that's mathematically impossible for skip to be less than zero so that the following condition will never happen:

    this.finish = function(check) {
        // skip will never possibly be less than zero
        var output = this.output + (skip < 0 ? alphabet[bits >> 3] : '') + (check ? '$' : '')
        this.output = ''
        return output
    }

If this is correct and I am not missing part of the originally meant logic, I think it would be safe to rewrite the code as such without any side effect:

    this.finish = function(check) {
        var output = this.output + (check ? '$' : '')
        this.output = ''
        return output
    }

What do you think?