Incrementing $n in the wrong order?

[deakjahn]

I don't use your actual PHP code directly but ported it to another language, but it seems to me that

https://github.com/ChristianRiesen/base32/blob/e8b7d85382e396b101d418844b997b4cd744cb8f/src/Base32.php#L104-L105

should be the other way around: first read the character, then increment $n. As it stands now, it will never read the first character of the input string.

Or, in one line:

$val += $chars[$n++];

[deakjahn]

Just another small idea, doesn't deserve a separate issue:

https://github.com/ChristianRiesen/base32/blob/e8b7d85382e396b101d418844b997b4cd744cb8f/src/Base32.php#L157-L163

I would modify this to:

$bitLen -= 8;
$decoded .= \chr($val >> $bitLen);
$val = $val & ((1 << $bitLen) - 1);

Actually, you could do something similar in the encoding part as well, $shift is similarly without any extra benefit there. Not really important, of course, this won't mean any performance difference these days. :-)

[ChristianRiesen]

Thanks for the inputs. I agree with the $shift part, that could be changed as it doesn't do anything else here. I think thats a remnant of a refactor that was left over, so thanks for spotting that.

As for the other one, you could combine it into one line, but it would hurt slightly in readability and also the classic ++$n versus $n++ confusion some people get. Here no matter what, the result would be the same. Again, mainly done for readability then anything else.

As for that being a bug, I don't think so, but I'm more then happy to be proven wrong. If it was a bug the library wouldn't pass the test vectors from the official RFC and people who use it, mainly for OTP and some crypto stuff, wouldn't be able to as it would output wrong data. Can you provide a case where it causes an error? Either Input, expected output, received output, or even a full unit test addition that breaks with the current implementation?

[deakjahn]

Well, as I mentioned, I didn't use it verbatim, I just ported it to Dart. And I test it with the very same vectors, of course, and for me it works with that modification. Even thinking about it, if you consider the first loop, $n will be incremented before you read the first character... How will you ever get the first? The decoding is different, you grab the $string[0] first but not in the encoding.

These are my current versions and they all pass the foobar tests. I modified the shift in both and also modified the writing into an if-else. Apart from slightly different variable names here and there and obvious differences due to the two languages having different features, it's the same except for that single difference of incrementing index after reading, nothing else:

String convert(List<int> input) {
  final padded = [...input, 0, 0, 0, 0, 0];
  final encoded = StringBuffer();

  for (int index = 0, bits = 0, value = 0; index < input.length || bits != 0;) {
    if (bits < 5) {
      value <<= 8;
      bits += 8;
      value += padded[index++];
    }
    if (index - (bits > 8).toInt() > input.length && value == 0) {
      bits -= 5;
      encoded.write('=');
    } else {
      bits -= 5;
      encoded.write(_base32Alphabet[value >> bits]);
      value &= (1 << bits) - 1;
    }
  }

  return encoded.toString();
}

Uint8List convert(String input) {
  final decoded = <int>[];
  if (input.isEmpty) return Uint8List(0);

  int value = _base32Alphabet[input[0]]!;
  for (int index = 0, bits = 5; index < input.length;) {
    if (bits < 8) {
      value <<= 5;
      bits += 5;
      final next = (++index < input.length) ? input[index] : '=';
      if (next == '=') index = input.length;
      value += _base32Alphabet[next]!;
    } else {
      bits -= 8;
      decoded.add(value >> bits);
      value &= (1 << bits) - 1;
    }
  }

  return Uint8List.fromList(decoded);
}