Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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";
  }
}
share|improve this question
add comment

1 Answer 1

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';
    }
    
share|improve this answer
    
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. –  Robottinosino Nov 8 '12 at 4:31
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.