6
\$\begingroup\$

Scenario: Return a String that contains only alphabets without spaces.

input = "A man, a plan, a canal: Panama"

output = "AmanaplanacanalPanama"

Can you please critique my code?

private String OnlyAlphabets(final String string) {
    StringBuilder sb = new StringBuilder();
    char ch;

    for (int i = 0; i < string.length(); i++) {
        ch = string.charAt(i);
        if (!Character.isAlphabetic(ch)) {
            continue;
        }
        sb.append(ch);
    }
    return sb.toString();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Separate from you current implementation, this is an excellent use case for regular expressions, which tend to be simpler, faster, and shorter than manual implementations, but do require some specialized learning. The regular expression for this would be just \S* (\S = anything not whitespace, * = repeated 0 or more times), for example. \$\endgroup\$ – gntskn Jun 7 at 11:36
  • \$\begingroup\$ @gntskn He also wants to get rid of anything not an alphabetic character. Also, a regular expression will be slower. \$\endgroup\$ – Glen Yates Jun 7 at 22:18
7
\$\begingroup\$
    char ch;

In general, we want to declare as late and at as small a scope as possible. You never use this outside the loop, so it could just be

        char ch = string.charAt(i);

inside the loop.

Even better, use the range-based/foreach style:

    for (char character : string.toCharArray()) {

Now you don't have to worry about i or charAt at all.

If you're using Java 8, you may want to look into the streaming API. E.g. combining this post and this post.

    return string.codePoints()
                 .filter( Character::isAlphabetic )
                 .collect( StringBuilder::new,
                           StringBuilder::appendCodePoint,
                           StringBuilder::append )
                 .toString();

I haven't tested this. I just combined the two solutions to similar problems in a way that I believe does what you want.

\$\endgroup\$
  • 1
    \$\begingroup\$ You should be wary of using toCharArray() when the intention is to only read the content. It creates full copy of the internal char array, which can be detrimental to memory performance, if the string is long. \$\endgroup\$ – TorbenPutkonen Jun 7 at 6:03
6
\$\begingroup\$

You code seems pretty straightforward. However, checking for true rather than false is probably more clear.

if (Character.isAlphabetic(ch)) 
{
   sb.append(ch);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I favor your check because there is just a single statement in the if, and no else. If there were more code after sb.append(ch);, continue would have been the best practice to avoid unnecessary nesting. \$\endgroup\$ – dfhwze Jun 7 at 4:57
  • \$\begingroup\$ @dfhwze what are your thoughts on a lot schools teaching only one return per function. Or the don't use continue or break in a loop. Fyi, I agree with your continue statement. \$\endgroup\$ – RockLeeroy Jun 7 at 5:10
  • 2
    \$\begingroup\$ I find the job of a teacher to make you think critically about things. They can advice and challenge us, but should not impose their standards :-) Unfortunately, these days, I feel .. hold on, let's not get political here. \$\endgroup\$ – dfhwze Jun 7 at 5:17
5
\$\begingroup\$

Your code looks good for very simple text. However, real life is much more complicated.

In languages other than English, there are characters that are combined from other characters. For example, the German Umlaut ä can be written either as \u00E4 or as an a, followed by the combining dots above, which is written a\u0308. Both representations look the same, yet your code treats them differently.

To get really international, you should read about the BreakIterator and how it breaks strings into "perceived characters". The Java type char does not represent such a perceived character, but only a small part of it.

To test whether such a perceived character (which is really a String in Java) is alphabetic, I guess if suffices to test whether that string contains an alphabetic code point (not char). Have a look at String.codePoints.

Learning how to handle international text properly takes time. Don't rush into it, and take the time needed. Here a little, there a little. It's impossible to get "the single correct algorithm" since languages of the world differ a lot in their interpretation of what a character really is. To take the first step in this journey, don't treat a string as a sequence of char, but as a sequence of code points. This alone makes your code handle most emojis correctly, and this simple step makes your code above-average already.

\$\endgroup\$
  • \$\begingroup\$ Even the letter a can be represented by different unicode code points :-) When dealing with strings, I would always distinguish common ANSI (windows-1252) vs Extended Unicode (UTF-*) when deciding the scope and complexity of a method. I don't think we should always be compliant to the latter. \$\endgroup\$ – dfhwze Jun 7 at 6:06
  • 1
    \$\begingroup\$ java.text.Normalizer and en.wikipedia.org/wiki/Unicode_equivalence seem relevant in this discussion. \$\endgroup\$ – TorbenPutkonen Jun 7 at 8:51
2
\$\begingroup\$

Your code looks good however, I have few suggestions.

private String OnlyAlphabets

the convention is to use camel case for method names, so it should be

private String onlyAlphabets

Although this is your sample program however, if such small utilities are required throughout the application then make them static and public. This will allow ease of access and since such utility does not modify the state of object so it is better to make them static.

public static String onlyAlphabets

Always check of null before doing further operations on your input string

if (Objects.isNull(string))
            return string;

If you are using java-8 or above then streams will surely make your code more compact with the below statement

return string.codePoints()
             .filter(Character::isAlphabetic)
             .collect(StringBuilder::new, 
                      StringBuilder::appendCodePoint, 
                      StringBuilder::append)
             .toString();
\$\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.