Checking if a sentence is considered valid

Question

Exercise: Checking if a sentence is considered valid under the following conditions:

• Numbers less than 13 are only allowed in their written form, e.g "twelve", but "14" is considered valid.
• Only 1 period marker is allowed and it must be the final character
• First letter must be uppercase
• number of quotation marks in the sentence must be even

if the sentence meets all above criteria, it is considered valid.

Here is my approach:

public class SentenceValidator implements ValidatesSentences {

  private static final int MINIMUM_ALLOWED_NUMBER = 13;

  public SentenceValidator() {
        //default (auto-generated without explicit declaration anyway)
  }

    /**
     * Takes a string sentence and returns if it meets the requirements
     * @param sentence -> The string sentence to analyze for validity
     * @return boolean -> if the string meets the validation requirements
     */
  @Override
  public final boolean isSentenceConsideredValid(String sentence) {
    String temporary = guardTheSentence(sentence);
    return temporary.length() > 1 && areConditionsValid(temporary);
  }

  private boolean areConditionsValid(String sentence) {
      return isTheFirstCharacterAnUppercaseLetter(sentence)
              && isTheCountOfQuotationMarksEven(sentence)
              && isTheOnlyPeriodMarkTheLastCharacterOfTheSentence(sentence)
              && isTheLowestNumberInTheSentenceGreaterThan13(sentence);
  }

  private String guardTheSentence(String sentence) {
    Objects.requireNonNull(sentence, "null is not an acceptable value");
    return sentence.trim();
  }

  private boolean isTheFirstCharacterAnUppercaseLetter(String sentence) {
    return Character.isLetter(sentence.charAt(0))
        && sentence.charAt(0) == sentence.toUpperCase().charAt(0);
  }

  private boolean isTheCountOfQuotationMarksEven(String sentence) {
    return (retrieveCountOfAGivenCharacter(sentence,'"') % 2) == 0;
  }

  private boolean isTheOnlyPeriodMarkTheLastCharacterOfTheSentence(String sentence) {
    return sentence.charAt(sentence.length() - 1) == '.' && retrieveCountOfAGivenCharacter(sentence,'.') == 1;
  }

  private int retrieveCountOfAGivenCharacter(String sentence, Character charToCount) {
    int counter = 0;
    for (char c : sentence.toCharArray()) {
      if (c == charToCount) {
        counter++;
      }
    }
    return counter;
  }

  /**
   * We don't need really need to care about words for the numbers, that is trivial All we want to
   * check is for all numeric value(s) within the string and ensure none are below 13
   * supports negative numbers using the optional (?:-)
   */
  private boolean isTheLowestNumberInTheSentenceGreaterThan13(String sentence) {
    String temp = sentence.replaceAll("[^-?0-9]+", " ");
    temp = temp.trim();

    if (temp.isEmpty()) {
        return true;
    }

    int lowestNumber = MINIMUM_ALLOWED_NUMBER;
    for (String number : temp.split(" ")) {
      lowestNumber = Math.min(lowestNumber, Integer.parseInt(number));
    }

    return lowestNumber >= MINIMUM_ALLOWED_NUMBER;
  }
}


public interface ValidatesSentences {

    boolean isSentenceConsideredValid(String sentence);
}

A jump out problem I see already is the NumberFormatException when number is greater than a max int, I should use a long/Big Integer there and handle a NumberFormatException.

Answer 1

What you have here is a series of boolean-result conditions that you want to apply to a given String.

1. The design can be expanded to allow List of conditions to be applied. This way you can add or remove conditions with minumal changes.

2. You can leverage Java 8 Predicate interface that fits the conditions you want to write. You can write at least some of the conditions as lambda expressions, saving you the need to write concrete clases

   List<Predicate<String>> conditions = new ArrayList<>();
   conditions.add((sentence) -> Character.isLetter(sentence.charAt(0))
           && sentence.charAt(0) == sentence.toUpperCase().charAt(0))
   
     private boolean areConditionsValid(String sentence) {
         return conditions.stream().allMatch(condition -> condition.test(sentence));
     }
   

Answer 2

Overall this looks great. You had a clear focus on readability and managed to choose decent method names. My only big question here would be: did you go a bit too far with putting everything in it's own method? The rest is more about choices and nitpicking.

I tried inlining a couple of methods which resulted in this implementation which I personally think is equally readable:

@Override
public final boolean isSentenceConsideredValid(String sentence) {
    Objects.requireNonNull(sentence, "null is not an acceptable value");
    String trimmedSentence = sentence.trim();
    return !trimmedSentence.isEmpty()
            && isTheFirstCharacterAnUppercaseLetter(trimmedSentence)
            && isTheCountOfQuotationMarksEven(trimmedSentence)
            && isTheOnlyPeriodMarkTheLastCharacterOfTheSentence(trimmedSentence)
            && isTheLowestNumberInTheSentenceGreaterThan13(trimmedSentence);
}

Now for the nitpicking, mostly focussed on performance issues and in no specific order:

sentence.toUpperCase().charAt(0) will turn the entire sentence to uppercase characters when you only want to check whether or not the first character is an uppercase letter. Why not just Character.isUppercase(sentence.charAt(0))?

To check for numbers you're already using a regex. Why not go all the way and search specifically for any "whitespace digit maybe second digit whitespace". That way you don't get into trouble with integer overflow. The downside is finding the correct regex to do exactly what you want (out of scope of this review to provide you with one).

Both the "count quotes even" and "check period only at end" loop through all the characters of the string. If performance is more important than readability you can combine those in a single for loop. The biggest gain here is that you can also exit early. If for example a string starts with a . you can immediatly return false without looping over the entire input string even once. Something like this (disclaimer untested):

    char[] input = sentence.toCharArray();
    if (input[input.length - 1] != '.') {
        return false;
    }
    boolean even = true;
    for (int i = 0; i < input.length -1; i++) {
        if(input[i]=='.'){
            return false;
        }
        if (input[i] == '"') {
            even = !even;
        }
    }
    return even;

Only worth doing if you value performance over readability though. If you only want to occasionaly check a String and it can take a couple of miliseconds your current implementation can be preferable from a maintenance point of view.

Is there a point to having an interface? What if you drop the interface and turn the other class into an actual utility class instead? That way you don't even need to create an instance of the class you're not going to need in the first place:

public class SentenceValidator {
    //private constructor to prevent wrongly creating instances of this class.
    private SentenceValidator () {} 

    public static boolean isValid(String sentence) {

Now you can call the check anywhere else by using SentenceValidator.isValid(sentence)

The only reason to have an interface like that is if you could have several different implementations that you want to decide at run time which to use.

Answer 3

In addition to both answers I'd like to propose changing the checking for numbers to use a Pattern matcher. This will be faster and easier.

Pattern p = Pattern.compile("-?[0-9]+");
Matcher m = p.matcher(sentence);
while (m.find()) {
    int n = Integer.parseInt(m.group());

    // check the integer
}

It tries to keep matching the regular expression, instead of replacing all values in the String. It should be more efficient as you don't have to replace the whole String. This also can fail on the first integer that is found that does not match your criteria.