5
\$\begingroup\$

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?

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

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;
}
| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

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.

| improve this answer | |
\$\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.