4
\$\begingroup\$

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));
  }
}
\$\endgroup\$
  • \$\begingroup\$ The class is Acronym, so no need to repeat that in the function name. Also, no need to add Relative and it is definitely the index for a character, so I would go for getNextIndex. Also, instead of isNextCharacterAnAcronymCharacter, I would go for isNextCharacterNeeded or something like that. \$\endgroup\$ – ChatterOne Aug 2 '16 at 8:39
  • \$\begingroup\$ @ChatterOne right you are - my explanation is that I was working off the implicit interface that exercism.io provided based on their test cases. \$\endgroup\$ – Jae Bradley Aug 2 '16 at 22:14
3
\$\begingroup\$

Taking the substring is not needed, if passing the index. As your loop is more or less a do-while kind: adding the first letter.

      index = nextIndexForAcronymChar(phrase, index + 1);
    }
    return sb.toString().toUpperCase();
  }

For the function there are two states: just having non-alphabetic or an uppercase letter. This can be handled with the same logic, just inverse conditions: forcing accepting a lowercase letter / skipping an uppercase letter.

  private static int nextIndexForAcronymChar(String phrase, int index) {
    boolean skipUpperCase = true; // index != 0?
    boolean acceptLowerCase = false; // index == 0?
    for (; index < phrase.length(); index++) {
      char currentChar = phrase.charAt(iindex);
      boolean lowerCase = Character.isLowerCase(currentChar);
      boolean upperCase = Character.isUpperCase(currentChar);
      if ((upperCase && !skipUpperCase)
          || (lowerCase && acceptLowerCase)) {
        return index;
      }
      skipUpperCase = upperCase;
      acceptLowerCase = !upperCase && !lowerCase;
    }
    return index;
  }

Back to the first loop. The function has two boolean states, that could be initialized by an additional boolean parameter or a condition like index == 0, so as to accept the first letter. And accepting a first letter in uppercase ("HTML", index == 1). I will not exercise my poor brain but leave it to you. So the loop might be rewritten, maybe in a for loop.

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

You don't need getRelativeIndexForNextAcronymCharacter, make it simpler: the goal here is to add characters into a StringBuilder where the current one and the next one meet a certain criteria. As such, you can just loop over the characters of the input String, memorize the current one and determine if the next one should be added to the acronym. Also, the first character will always be added to the acronym, there's no need to have a special check for it.

public static String generate(final String phrase) {
    final StringBuilder sb = new StringBuilder();
    char currentChar = phrase.charAt(0);
    sb.append(currentChar);
    for (int i = 1; i < phrase.length(); i++) {
        final char nextChar = phrase.charAt(i);
        if (isNextCharacterAnAcronymCharacter(currentChar, nextChar)) {
            sb.append(nextChar);
        }
        currentChar = nextChar;
    }
    return sb.toString().toUpperCase();
}

You're right that the method names are a bit long (isNextCharacterAnAcronymCharacter), however it is fairly descriptive in what it does and I don't find it too long. I wouldn't change it.

\$\endgroup\$

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.