Calculating the distance between two letters

Question

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);
        }
    }

}

Answer 1

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);
}

Answer 2

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.

Answer 3

A few additional points on top of the other excellent reviews.

Think twice before using checked exceptions

Checked exceptions can make a method harder to use than necessary. Calling a method that throws a checked exception requires special treatment, which can increase complexity.

In this example, using a checked exception feels forced, and probably not necessary. By the name of the method, it only makes sense to call it with characters that are letters. If you have untrusted input, it would be better to validate it before calling this method, as it wouldn't make sense to pass non-letters to it.

As such, I suggest to use IllegalArgumentException instead of a custom checked exception:

public int getDistanceBetweenTwoLetters(char firstChar, char secondChar) {
    if (!isLetter(firstChar)) {
        throw new IllegalArgumentException("Not a letter: " + firstChar);
    }
    if (!isLetter(secondChar)) {
        throw new IllegalArgumentException("Not a letter: " + secondChar);
    }
    // borrowed code from @JS1
    return Character.toLowerCase(secondChar) - Character.toLowerCase(firstChar);
}

Naming

The "get" practice is most commonly used for simple getters. In this example there is not exactly trivial logic, so I'd recommend to use a different prefix, for example "calculate" or "calc". And since the method parameters imply two letters, I think you can drop "BetweenTwoLetters" from the name, to get the short and sweet:

public int calculateDistance(char firstChar, char secondChar) {
    // ...
}

Working with enums

Will it make sense for BeforeOrAfter to have other values than "Before" and "After"? Probably not. It will be confusing too. In general, it's pretty fishy when the name of an enum is formed literally from the names its possible values.

This declaration gives a hint:

BeforeOrAfter direction = ...

That's right, the enum is about directions, so Direction would have been a better name.

Lastly, enum values are immutable singletons, so no need for .equals to check for equality:

if(direction.equals(BeforeOrAfter.BEFORE)){

Use the == operator:

if (direction == BeforeOrAfter.BEFORE) {