Check if a string is a valid english word, and convert to a valid english word

Question

This is an interview question.
For the purpose of the question, a valid English word is a string that begins with an uppercase English letter, and the rest of the string contains only lowercase English letters. I needed to implement two functions:

1. A function that receives a String and returns true if the String is a valid English word, and false otherwise.

2. A function that receives a String and convert it to a valid english word. That means that the function removes non-English characters from the string, and also lowercase\uppercase letters that are in the wrong place. Examples:

   convertToValidString("A") = "A"                
   convertToValidString("a") = ""                 
   convertToValidString("Ab") = "Ab"               
   convertToValidString("AB") = "A"                    
   convertToValidString("Ab5eA") = "Abe"            
   convertToValidString("aAd") = "Ad"               
   convertToValidString("a6d") = "" 
   

Below is the code I wrote, please tell me what do you think. My emphasis is mainly about efficiency (run time + short code), and less about coding conventions.

public static boolean isValidString(String s){
    int len = s.length();

    if (len == 0) return true;
    if ( (s.charAt(0) < 65) || (s.charAt(0) > 90) ) return false;
    for (int i = 1; i < len; i++){
        if ( (s.charAt(i) < 97) || (s.charAt(i) > 120) ) return false;
    }

    return true;
}

public static String convertToValidString(String s) {
    StringBuffer ans = new StringBuffer();
    int len = s.length();
    int i = 0;

    while ( (i < len) && ((s.charAt(i) < 65) || (s.charAt(i) > 90)) )
        i++;

    if (i < len){
        ans.append(s.charAt(i));
        i++;
        for (; i < len; i++){
            if ( (s.charAt(i) >= 97) && (s.charAt(i) <= 120) ) 
                ans.append(s.charAt(i));
        }
    }

    return ans.toString();
}

Answer 1

You should avoid statements on the same line as an if.
So for example

if (len == 0)
    return true;

instead of

if (len == 0) return true;

Also, I find that the (s.charAt(0) < 65) || (s.charAt(0) > 90) are really hard to understand as they use magic numbers.

But, most importantly, are you sure your code is responding to all the cases ?

Let's create some unit test with JUnit and AssertJ :

class TestTestString {
    @Test
    void isValidString_shouldAcceptSimpleWord() {
        assertThat(TestString.isValidString("Hello")).isTrue();
    }

    @Test
    void isValidString_shouldAcceptVeryLongWord() {
        assertThat(TestString.isValidString("Thisincrediblylongwordisylbidercnilongz")).isTrue();
    }

    @Test
    void isValidString_shouldAcceptSingleUppercaseString() {
        String[] values = { "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N" };
        for (String s : values) {
            assertThat(TestString.isValidString(s)).isTrue();
        }
    }

    @Test
    void isValidString_shouldAcceptStringWithDiacritics() {
        assertThat(TestString.isValidString("Résumé")).isTrue();
    }

    @Test
    void isValidString_shouldRefuseStringThatStartWithALowerCaseLetter() {
        assertThat(TestString.isValidString("apropos")).isFalse();
    }

}

Without much surprise, the isValidString_shouldAcceptStringWithDiacritics is failing... but so is the isValidString_shouldAcceptVeryLongWord because the 120 isn't the good number !
Never forget to do some unit tests :) (you should also add some tests for numbers and special characters)

To conclude, if I were you, I'd get rid of the current test altogether and use the Character methods that are nicer lookings, namely isUpperCase and isLowerCase :

public static boolean isValidString(final String s){
        int len = s.length();

        if (len == 0)
            return true;
        if (!Character.isUpperCase(s.charAt(0)))
            return false;
        for (int i = 1; i < len; i++) {
            if (!Character.isLowerCase(s.charAt(i)))
                return false;
        }

        return true;
    }

Now, if you rerun the tests, they should be all green.

You can also use the same methods for your convert function ;)

Answer 2

Way too many magic numbers: 65, 90, 97, 120. Instead, use literal constants that the reader can understand without consulting an ASCII chart, like:

s.charAt(i) < 'A' || s.charAt(i) > 'Z'

Store your character in a local variable, instead of calling .charAt() on the same character 3 times in the last loop.

Consider using Character::isUpperCase() and Character::isLowerCase() functions.

Use StringBuilder instead of StringBuffer for efficiency.