Open joedrago opened 10 years ago
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...
Generally speaking, this should be rewritten as a TransformStream. It's OOOOLD.
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.
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?
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.