1
\$\begingroup\$

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();
}
\$\endgroup\$
1
\$\begingroup\$

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 ;)

\$\endgroup\$
0
\$\begingroup\$

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.

\$\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.