Acronym Generation

Problem

• Implement a relatively naive acronym generation. I was trying to implement a solution without using regex.

• Portable Network Graphics => PNG.

• Ruby on Rails => ROR.

• HyperText Markup Language => HTML.

• Complementary metal-oxide semiconductor => CMOS.

• Origin is from code exercise site, exercism.io.

Implementation

• My implementation has looooonnnggggg method names (I'm looking at you getRelativeIndexForNextAcronymCharacter and isNextCharacterAnAcronymCharacter). I'd appreciate any naming suggestions...

• I know that the Character.isAlphabetic check in generate is redundant except when index = 0. Is there a better way to deal with the case where the first letter should be included in the acronym?

public class Acronym {
  public static String generate(final String phrase) {
    final StringBuilder sb = new StringBuilder();
    int index = 0;
    while (index < phrase.length()) {
      if (Character.isAlphabetic(phrase.charAt(index))) {
        sb.append(phrase.charAt(index));
      }

      index += getRelativeIndexForNextAcronymCharacter(phrase.substring(index));
    }
    return sb.toString().toUpperCase();
  }

  private static int getRelativeIndexForNextAcronymCharacter(final String substring) {
    for (int i = 0; i < substring.length() - 1; i++) {
      final char currentChar = substring.charAt(i);
      final char nextChar = substring.charAt(i + 1);
      if (isNextCharacterAnAcronymCharacter(currentChar, nextChar)) {
        return i + 1;
      }
    }
    return substring.length();
  }

  private static boolean isNextCharacterAnAcronymCharacter(final char currentChar, final char nextChar) {
    return (!Character.isAlphabetic(currentChar) && Character.isAlphabetic(nextChar)) ||
           (Character.isLowerCase(currentChar) && Character.isUpperCase(nextChar));
  }
}

Who needs math?

for (int i = 0; i < substring.length() - 1; i++) {
      final char currentChar = substring.charAt(i);
      final char nextChar = substring.charAt(i + 1);
      if (isNextCharacterAnAcronymCharacter(currentChar, nextChar)) {
        return i + 1;
      }
    }

This does more math than necessary. Consider

for (int i = 1; i < substring.length(); i++) {
      final char previousChar = substring.charAt(i - 1);
      final char currentChar = substring.charAt(i);
      if (isCurrentCharacterAnAcronymCharacter(previousChar, currentChar)) {
        return i;
      }
    }

Previously you did a subtraction (might get optimized out by the compiler) and two additions on every loop. This way you only do one subtraction.

A side benefit is that now you are returning the current character rather than the next character. Notionally that makes more sense to me.

But I actually think you are better off without this method.

Why use two methods to loop over one string?

int index = 0;
    while (index < phrase.length()) {
      if (Character.isAlphabetic(phrase.charAt(index))) {
        sb.append(phrase.charAt(index));
      }

      index += getRelativeIndexForNextAcronymCharacter(phrase.substring(index));
    }

You don't actually need getRelativeIndexForNextAcronymCharacter. If you pull that logic into this method, it actually leaves this method about as simple as it is now.

// initialize to non-alphabetic so the first alphabetic character gets used
    char previous = '.';
    for (char current : phrase.toCharArray()) {
        if (isNextCharacterAnAcronymCharacter(previous, current)) {
            sb.append(current);
        }

        previous = current;
    }

And that gets rid of the entire getRelativeIndexForNextAcronymCharacter method. Which incidentally gets rid of the math around i.

Initializing previous to a non-alphabetic character is a bit hacky. If you prefer to avoid that, you can use an extra pair of variables instead.

Hack free but complicated

char previous = '.';
    boolean previousAlphabetic = false;
    for (char current : phrase.toCharArray()) {
        final boolean currentAlphabetic = Character.isAlphabetic(current);
        if ((!previousAlphabetic && currentAlphabetic)
            || (Character.isLowerCase(previous) && Character.isUpperCase(current))) {

            sb.append(current);
        }

        previous = current;
        previousAlphabetic = currentAlphabetic;
    }

That's a little more complicated, but it eliminates the isNextCharacterAnAcronymCharacter method. It also calls Character.isAlphabetic fewer times per iteration, as it copies the previous call instead.

I prefer to put operators (e.g. ||) at the beginning of a line rather than the end. This makes it easier to see that a line is a continuation of a previous line when looking at the beginning of the line. It's generally easier to quickly scan down the left, mostly fixed position side than the right, variable length side.

I think that I prefer the hacky but simple solution to this one. Both seem reasonable if imperfect.

StackExchange Code Review Q#136611, answer score: 4