Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I completed a sample programming challenge and wanted to find out how I could make it more efficient. I'm trying to get better at writing more efficient code, so I would be interested in hearing ideas on how I can make the code more efficient. I ran it, and it works, just curious if there are inefficiencies that some of the seasoned developers could point out.

One thing I know seems wasteful is the continuous polling of the before or after to determine if I should increment or decremented the distance, but couldn't think of a better way right off hand. Tried a ternary statement, but I couldn't get it to work.

public class Main {

    /*Write a method to calculate the distance between two letters (A-Z, a-z,
     case insensitive). The input to the method is two char primitives.
     If either char is not A-Za-z, throw an AlphabetException. Distance in this case is defined
     as the number of letters between firstChar and secondChar.
     For example, the distance between 'a' and 'c' is 2, and the distance
     between 'c' and 'a' is -2. Counting is done alphabet wise, so the
     distance between 'a' and 'z' is 25, while the distance between 'z' and 'a'
     is -25. The distance calculation should be case insensitive, so calclating
     the distance between 'A' and 'c' yields the same result as the distance
     between 'a' and 'c'. If the firstChar is equal to the secondChar, the
     distance is considered 0.
     */
    public int getDistanceBetweenTwoLetters(char firstChar, char secondChar) throws AlphabetException {
        if(!isLetter(firstChar)){
            throw new AlphabetException(firstChar+" is not part of the alphabet");
        }
        if(!isLetter(secondChar)){
            throw new AlphabetException(secondChar+" is not part of the alphabet");
        }
        String firstStringValue = String.valueOf(firstChar).toUpperCase();
        Character firstCharacter = firstStringValue.charAt(0);

        String secondStringValue = String.valueOf(secondChar).toUpperCase();
        Character secondCharacter = secondStringValue.charAt(0);

        BeforeOrAfter direction = secondCharBeforeOrAfter(firstCharacter, secondCharacter);
        int distance = 0;
        Character referenceCharacter = firstCharacter;
        boolean foundIt = referenceCharacter.equals(secondCharacter);
        while(!foundIt){
            if(direction.equals(BeforeOrAfter.BEFORE)){
                distance--;
            }else{
                distance++;
            }
            referenceCharacter = getCharacterWithRespectToInputChar(direction, referenceCharacter);
            foundIt = referenceCharacter.equals(secondCharacter);
        }
        return distance;
    }

    private BeforeOrAfter secondCharBeforeOrAfter(Character firstChar, Character secondChar){
        return secondChar.compareTo(firstChar) <1?BeforeOrAfter.BEFORE:BeforeOrAfter.AFTER;
    }

    private boolean isLetter(Character inputChar){
        return Pattern.matches("[a-zA-Z]+", inputChar.toString());
    }

    private Character getCharacterWithRespectToInputChar(BeforeOrAfter beforeOrAfter, Character inputCharacter) {
        switch (inputCharacter) {
            case 'A':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? null : 'B';
            case 'B':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'A' : 'C';
            case 'C':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'B' : 'D';
            case 'D':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'C' : 'E';
            case 'E':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'D' : 'F';
            case 'F':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'E' : 'G';
            case 'G':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'F' : 'H';
            case 'H':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'G' : 'I';
            case 'I':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'H' : 'J';
            case 'J':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'I' : 'K';
            case 'K':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'J' : 'L';
            case 'L':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'K' : 'M';
            case 'M':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'L' : 'N';
            case 'N':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'M' : 'O';
            case 'O':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'N' : 'P';
            case 'P':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'O' : 'Q';
            case 'Q':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'P' : 'R';
            case 'R':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'Q' : 'S';
            case 'S':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'R' : 'T';
            case 'T':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'S' : 'U';
            case 'U':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'T' : 'V';
            case 'V':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'U' : 'W';
            case 'W':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'V' : 'X';
            case 'X':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'W' : 'Y';
            case 'Y':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'X' : 'Z';
            case 'Z':
                return beforeOrAfter.equals(BeforeOrAfter.BEFORE) ? 'Y' : null;
            default:
                return null;
        }
    }

    private enum BeforeOrAfter {
        BEFORE, AFTER;
    }

    private class AlphabetException extends Exception {
        public AlphabetException(String message){
            super(message);
        }
    }

}
share|improve this question
    
The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask. –  Jamal yesterday

2 Answers 2

Way too complicated

Everything after the two isLetter() checks is extremely complicated and unnecessary. You can just subtract the two character values, like this:

public int getDistanceBetweenTwoLetters(char firstChar, char secondChar) throws AlphabetException {
    if(!isLetter(firstChar)){
        throw new AlphabetException(firstChar+" is not part of the alphabet");
    }
    if(!isLetter(secondChar)){
        throw new AlphabetException(secondChar+" is not part of the alphabet");
    }
    return Character.toLowerCase(secondChar) - Character.toLowerCase(firstChar);
}
share|improve this answer
private getCharacterWithRespectToInputChar() { ... }

Hmms... why do you need this and the complicated switch statement?

It will be much shorter, and likely more efficient if you simply have this:

private static final String ENGLISH_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

And then use indexOf() to determine their absolute positioning within the String. Obtaining the relative positioning as your challenge requires is to simply subtract one value from the other.

Also, this can be done as a static method, so you do not need to instantiate the class to use getDistanceBetweenTwoLetters(char, char). It's nice to have a custom Exception class, but you can probably just rely on good ol' IllegalArgumentException here.

private boolean isLetter(Character inputChar){
    return Pattern.matches("[a-zA-Z]+", inputChar.toString());
}

It is recommended to make the Pattern instance static as it can be safely reused to create Matcher instances. In any case, the regex here should be "\\p{Alpha}" proper way here is to rely on Character.isLetter() as long as you are sure that you only need to handle US-ASCII characters.

In short, your solution shows the usage of enums, switch statements, loops, regex and custom Exception implementation, but these are redundant for the challenge at hand.

share|improve this answer

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.