Determine if a sentence is a pangram

This definition is taken from Wikipedia:


A pangram (Greek: παν γράμμα, pan gramma, "every letter") or holoalphabetic sentence for a given alphabet is a sentence using every letter of the alphabet at least once. Pangrams have been used to display typefaces, test equipment, and develop skills in handwriting, calligraphy, and keyboarding.

I found a related code challenge here. It was in Python, so I tried to code this program in Java.

I took two steps:

• Find if the String is a pangram

• If a string is not a pangram, then find the missing letters

Please review the approach I have taken.

Pangram

```
import java.util.Set;
import java.util.TreeSet;

public class Pangram {
private static final int ASCII_VALUE_OF_SMALL_CASE_CHAR_A = 97;
private static final int ASCII_VALUE_OF_SMALL_CASE_CHAR_Z = 122;
private Set distinctCharsInInputStringSortedAlphabetically = new TreeSet();

public Pangram(final String inputString) {
addUniqueAlphabetsToSet(inputString);
}

public boolean isPangram() {
return distinctCharsInInputStringSortedAlphabetically.size() == 26;
}

private void addUniqueAlphabetsToSet(final String inputString) {
for (Character character : inputString.toLowerCase().toCharArray()) {
if ((int) character >= ASCII_VALUE_OF_SMALL_CASE_CHAR_A
&& (int) character getMissingAlphabets() {
Set missingAlphabets = new TreeSet();
if (!isPangram()) {
char alphabet_a = 'a';
int asciiValue = (int) alphabet_a;
for (Character alphabetsInInput : distinctCharsInInputStringSortedAlphabetically) {
do {
if ((int) alphabetsInInput > asciiValue) {
missingAlphabets.add((char)asciiValue);
}
asciiValue++;
} while ((int) alphabetsInInput >= asciiValue);

}

if(asciiValue <=ASCII_VALUE_OF_SMALL_CASE_CHAR_Z){

The public interface of the class is nice and small. The code is laid out well and follows Java coding conventions. The tests have reasonable coverage.

There is a lot of unnecessary looping and casting in your code.
Here's a more concise version that passes your tests:

public class Pangram {

    private final Set lettersRemaining = new HashSet<>();

    public Pangram(String s) {
        for (char ch = 'a'; ch  getMissingAlphabets() {
        return new HashSet<>(lettersRemaining);
    }
}

Suggestions...

-
Call it PangramCandidate rather than Pangram because it is misleading to have a non-pangram typed as a Pangram. As an analogy, you wouldn't expect String to have an isString() method.

-
The very long variable name distinctCharsInInputStringSortedAlphabetically is referenced in several places which makes it tedious to read the code. I think you should find a briefer way of expressing what that variable represents.

-
I do like your long test method names. However, the test method names starting with checkPangram_Test1..4 aren't very explanatory. Can you explain what exactly they are testing?

-
The term MissingAlphabets seems awkward to me. I think you mean MissingLetters or MissingCharacters.

-
In the tests, you can extract convenience methods for asserting whether a string is a pangram or not. This would reduce the amount of repetition in the test code.

public static boolean isPangram(String s) {
    return new Pangram(s).isPangram();
}

public static void assertIsAPangram(String s) {
    assertTrue(isPangram(s));
}

public static void assertIsNotAPangram(String s) {
    assertFalse(isPangram(s));
}

I would actually suggest adding the isPangram(String s) convenience method to the PangramCandidate class because it will save callers time if that's all they need.

-
Also, you might think about how to cater for foreign characters (e.g. é) or different alphabets, say Cyrillic or even Spanish. This checker works for English only.

-
One of the lines in your test is very long due to a long String. I would suggest splitting it into two lines joined with +.

-
If you use the Google Guava library you can create test sets more concisely, e.g.

Sets.newHashSet('a', 'e', 'g', 'h', 'l', 'm', 'q', 't', 'w', 'y', 'z')

-
In your code, the conditional:

if ((int) character >= ASCII_VALUE_OF_SMALL_CASE_CHAR_A
        && (int) character <= ASCII_VALUE_OF_SMALL_CASE_CHAR_Z) {

can be expressed more succinctly as:

if (character >= 'a' && character <= 'z') {

There is no need to cast to an int and there is little value in expressing these characters as constants. It's not like they are going to change, and 'a' is much quicker to read and comprehend than a wordy explanation.

StackExchange Code Review Q#71930, answer score: 9