CodingBat countYZ

Question

In reference to the problem here

Given a string, count the number of words ending in 'y' or 'z' -- so the 'y' in "heavy" and the 'z' in "fez" count, but not the 'y' in "yellow" (not case sensitive). We'll say that a y or z is at the end of a word if there is not an alphabetic letter immediately following it. (Note: Character.isLetter(char) tests if a char is an alphabetic letter.)

I wrote down the following solution.

public int countYZ(String str) {
  final int len = str.length();
  int res = 0;
  if (len  == 0) return 0;

  for (int i = 0; i < str.length(); i++) {
    if (str.substring(i, i + 1).equalsIgnoreCase("y") || str.substring(i, i + 1).equalsIgnoreCase("z"))
      if (((i < len - 1) && !(Character.isLetter(str.charAt(i + 1)))) || i == len -1)
          res += 1;
  }
  return res;
}

Please feel free to review it, It looks a bit messy I know, is there a cleaner and easier solution to this?

Answer 1

You defined len, yet you also wrote str.length() in the loop condition.

Renaming res to count would result in more readable code, in my opinion.

if (len == 0) return 0; is an unnecessary special case. You should remove it, and just let the general case do its work.

Never omit the "optional" braces in a multi-line if statement. It's error-prone and makes the code hard to follow.

Both of your if conditions are too verbose. I would write them like this:

public int countYZ(String str) {
  final int len = str.length();
  int count = 0;

  for (int i = 0; i < len; i++) {
    char c = str.charAt(i);
    if (c == 'Y' || c == 'y' || c == 'Z' || c == 'z') {
      if (i + 1 == len || !Character.isLetter(str.charAt(i + 1))) {
        count++;
      }
    }
  }
  return count;
}

Personally, I would prefer to look for the ends of words first, for slightly more compact code:

public int countYZ(String str) {
  int count = 0;    
  for (int i = 1; i <= str.length(); i++) {
    if (i == str.length() || !Character.isLetter(str.charAt(i))) {  // End of word
      char prev = str.charAt(i - 1);
      if (prev == 'Y' || prev == 'y' || prev == 'Z' || prev == 'z') {
        count++;
      }
    }
  }
  return count;
}

Bonus solution

This might be considered "cheating" for CodingBat, but using regular expressions would lead to concise code whose purpose is easily inferred at a glance.

public int countYZ(String str) {
  java.util.regex.Pattern yzEnd = java.util.regex.Pattern.compile("[YyZz](?!\\p{IsAlphabetic})");
  java.util.regex.Matcher matcher = yzEnd.matcher(str);
  int count;
  for (count = 0; matcher.find(); count++);
  return count;
}

Answer 2

Your early exit test for zero length is superfluous, and should be deleted.

Use {} curly braces with if, even for a single statement body. It's an aid to folks reading your code, and it will prevent one class of bugs when someone later inserts a line of code.

The identifier res for result is nice enough, but consider renaming it to something more descriptive, perhaps count.

Your loop body is nice enough, but consider writing a helper, a predicate for whether character matches the target set of [yz]. Consider giving the name isEndOfWord to that final boolean expression.