3
\$\begingroup\$

I am trying to learn some Java on my own, and I am tackling some "programming challenge" problems to practice the little I have learnt so far.

Could anybody constructively critique this beginner's effort (source on Bitbucket)?

public class BinaryCode {
  public String[] decode(String message) {
    int messageLength = message.length();
    return new String[] { decodeMessage(message, messageLength, 0),
        decodeMessage(message, messageLength, 1) };
  }
  private String decodeMessage(String encodedMessage, int messageLength,
      int assumedFirstDigit) {
    StringBuilder decodedMessage = new StringBuilder(messageLength);
    int nextDecodedDigit = assumedFirstDigit;
    for (int i = 0; i < messageLength; i++) {
      if (nextDecodedDigit != 0 && nextDecodedDigit != 1) {
        return "NONE";
      }
      decodedMessage.append(nextDecodedDigit);
      int encodedDigit = encodedMessage.charAt(i) - '0';
      int decodedDigit = decodedMessage.charAt(i) - '0';
      nextDecodedDigit = encodedDigit - decodedDigit;
      if (i > 0) {
        nextDecodedDigit -= decodedMessage.charAt(i - 1) - '0';
      }
    }
    int checkValue = decodedMessage.charAt(messageLength - 1) - '0';
    if (messageLength > 1) {
      checkValue += decodedMessage.charAt(messageLength - 2) - '0';
    }
    if (checkValue == encodedMessage.charAt(messageLength - 1) - '0') {
      return decodedMessage.toString();
    }
    return "NONE";
  }
}
\$\endgroup\$
0
1
\$\begingroup\$

These critiques are mostly style and syntax related, and do not address your algorithm (you could probably find the simplest possible algorithm with Google anyway).

  • make decode and decodeMessage static
  • explicitly hide the constructor for BinaryCode, e.g.

    // hide constructor
    private BinaryCode() {}
    
  • remove unneseccary - 0 from encodedDigit and decodedDigit declarations
  • let decodeMessage calculate messageLength (fewer parameters are nearly always better)
  • avoid using magic numbers and string literals (-1, -2, "NONE", etc...)
  • create method lastChar and call it instead of messageLength - 1 e.g. int checkValue = lastChar(decodedMessage) - '0';

    private static char lastChar(CharSequence sequence) {
        int lastIndex = sequence.length() - 1;
        return sequence.charAt(lastIndex);
    }
    
  • replace decodedMessage.charAt(messageLength - 2) and decodedMessage.charAt(i - 1) with previouslyDecodedChar e.g.

    char previouslyDecodedChar = '\0';
    for (int i = 0; i < messageLength; i++) {
        // ...
        if (i > 0) {
            nextDecodedDigit -= previouslyDecodedChar - '0';
        }
        previouslyDecodedChar = decodedMessage.charAt(i);
    }
    int checkValue = lastChar(decodedMessage) - '0';
    if (messageLength > 1) {
        checkValue += previouslyDecodedChar - '0';
    }
    
\$\endgroup\$
1
  • \$\begingroup\$ Thanks very much for your comments. I really appreciated them. - '0' is not redundant. The method signatures were mandated by the exercise, and I "think" the instantiability is too, so I could not hide the ctor. \$\endgroup\$ – Robottinosino Nov 8 '12 at 4:31

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.