4
\$\begingroup\$

I wrote a function to decode GSM 7-Bit encoded messages as defined in the 3GPP standard. Example available here. I found a tool to help me check my answers here. I used the charset from here.

public String decode(final byte[] data) {
    if (data == null) {
        return null;
    }

    final ByteArrayOutputStream bos = new ByteArrayOutputStream();
    for (int i = 0, counter = 0; i < data.length; i++, counter++) {
        final byte current = data[i];
        final byte previous = i == 0 ? 0 : data[i - 1];

        if (counter == 7) {
            counter = 0;
        }

        final int shiftedCurrent = (current << counter) & 0x7F;
        final int shiftedPrevious = (previous & 0xFF) >> (8 - counter);
        bos.write(shiftedCurrent | shiftedPrevious);

        if (counter == 6) {
            bos.write((current & 0xFF) >> 1);
        }
    }

    try {
        final String result = bos.toString("X-Gsm7Bit");
        return result.endsWith("\r") ? result.substring(0, result.length() - 1) : result;
    } catch (final UnsupportedEncodingException e) {
        throw new AsnDecodeException("Cannot get charset: X-Gsm7Bit");
    }
}

Tests:

@Test
public void decodeTests() throws DecoderException {
    assertThat(decode(Hex.decodeHex("f4f29c9e769f1b".toCharArray()))).isEqualTo("testing");
    assertThat(decode(Hex.decodeHex("31D98C56B3DD70".toCharArray()))).isEqualTo("12345678");
    assertThat(decode(Hex.decodeHex("6176D94DAFCB1B".toCharArray()))).isEqualTo("alentur");
    assertThat(decode(Hex.decodeHex("61F79B8E2ECB5B657CB80D679701E77638CD768DDF6D".toCharArray()))).isEqualTo("another-example@gmail.com");
}

I feel like I could have done this with fewer shifts or & checks, but the thing I really don't like is my counter variable that I reset at 7. Any advice on how to improve this code?

\$\endgroup\$
1
\$\begingroup\$

thing I really don't like is my counter variable that I reset at 7

may be just keep incrementing the counter as counter = (counter + 1) % 8 instead of keeping it in the for loop.

Any advice on how to improve this code?

this may be just me, but bit operations are very mysterious so I'd like to have a variable for 0x7F & 0xFF to understand what's to special

Also, I'm not really sure why counter == 6 leads to the additional bit. It would help if you could take it out into a function that gets called.

result.endsWith("\r") seems a bit odd. Is that a specific requirement. If so, do you also need to handle \n ?

throw new AsnDecodeException("Cannot get charset: X-Gsm7Bit"); when throwing exceptions, I'd recommend adding some context on the input data leading to the error message. It helps a lot when

Don't forget to test for bad input data like

  • ""
  • null
  • "zz"
  • "123456789ø"

I'd also just recommend creating a helper class and using hamcrest matchers to make the testing more readable i.e. assertThat(hex("f4f29c9e769f1b"), is("testing"))

\$\endgroup\$
  • \$\begingroup\$ I am trying to make this piece of code as fast as possible, and I am not sure I like doing % 8 each loop. 0x7F is a 7 bit mask - 0111 1111 and 0xFF is 8 bit mask - 1111 1111. I think I messed up with \r, there is a paragraph here (etsi.org/deliver/etsi_ts/123000_123099/123038/10.00.00_60/…), chapter: '6.1.2.3.1' about carriage returns, I think I should do this only when length == 7. Hex conversion method is from apache, no need to test that, just the output. I like the test advice, thanks! \$\endgroup\$ – alturkovic Feb 22 '17 at 8:30
  • \$\begingroup\$ If the main goal is to be fast, I'd say avoid worrying about what's you like or don't like. Just go with whatever gives you the speed. Placing those bit masks in variables would make the code a lot more readable. I'd still recommend verifying that you are receiving hex data as input. a possible alternate to counter = (counter +1 ) % 8could be ` counter = (counter + 1) & 0x8;` \$\endgroup\$ – Rishi B Feb 22 '17 at 23:21

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.