7
\$\begingroup\$

The following code checks if a string is a pangram or not:

public static class StringExtensions
{
    public static IEnumerable<char> EnglishAlphabet
    {
        get
        {
            for (char i = 'a'; i <= 'z'; i++)
            {
                yield return i;
            }

            yield break;
        }
    }

    /// <summary>
    /// Check if the source string is a pangram.
    /// </summary>
    /// <remarks>
    /// The default dictionary is the english one.
    /// </remarks>
    public static bool IsPangram(this string source)
    {
        return source.IsPangram(EnglishAlphabet);
    }

    /// <summary>
    /// Check if the source string is a pangram.
    /// </summary>
    /// <remarks>
    /// A pangram is a string that uses every letter in the alphabet.
    /// </remarks>
    public static bool IsPangram(this string source, IEnumerable<char> alphabet)
    {
        var lowerAlphabet = new string(alphabet.ToArray()).ToLower();
        IDictionary<char, bool> alphabetCharacters = lowerAlphabet.ToDictionary(c => c, c => false);
        var lowerSource = source.ToLower();

        foreach(char character in lowerSource)
        {
            alphabetCharacters[character] = true;
        }

        return !alphabetCharacters.Values.Contains(false);
    }
}

Do you see anything wrong/weird in it? Any feedback (style, naming, performance) is more than welcome.

\$\endgroup\$
1
  • \$\begingroup\$ no reason to recreate EnglishAlphabet nor tolower \$\endgroup\$
    – paparazzo
    Sep 28, 2016 at 2:36

7 Answers 7

9
\$\begingroup\$

Use a set instead of a dictionary

The current algorithm sets the value to true for each character it finds, and at the end it checks if there is anything still unset. This is a bit tedious, and inefficient. Checking if any value is false is an \$O(n)\$ operation.

A simpler and more natural solution would be using a set data structure (HashSet in C#). You could add characters that you found, and in the end check the size of the set, and compare it with the size of the alphabet.

If the input may contain letters not in the alphabet, then before adding letters to the found set, don't forget to check the letter is actually part of the alphabet. (Thanks @tym32167 for the reminder.)

Use a simple array instead of a dictionary

If the alphabet is always the English alphabet, then an interesting simple alternative is using a simple boolean array of size 'z' - 'a' + 1. As you iterate over the letters, you can derive the array index to use by letter - 'a'. It will be simpler and more storage efficient than using a dictionary.

Generate the alphabet once

The English alphabet is regenerated on each call of IsPangram. This is unnecessary, as it never changes between calls. You could generate it once and reuse.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I didn't think of using a set actually. And now the code is way more elegant also. Thanks. \$\endgroup\$ Sep 27, 2016 at 21:35
  • \$\begingroup\$ And don't wait until the end. If it is at 26 then stop. \$\endgroup\$
    – paparazzo
    Sep 28, 2016 at 2:38
  • \$\begingroup\$ You could add characters that you found => don't forget to add only those characters, which included in your alphabet. \$\endgroup\$
    – tym32167
    Sep 28, 2016 at 11:04
7
\$\begingroup\$

Just a small point:

public static IEnumerable<char> EnglishAlphabet
{
    get
    {
        for (char i = 'a'; i <= 'z'; i++)
        {
            yield return i;
        }

        yield break;
    }
}

Strings already implement IEnumerable<char>. You could replace all this code with:

public readonly static IEnumerable<char> EnglishAlphabet = "abcdefghijklmnopqrstuvwxyz";
\$\endgroup\$
3
  • 1
    \$\begingroup\$ This answer doesn't really help. The OP already knows that because he uses such a loop in his code foreach(char character in lowerSource) and I think it's easier to create an alphabet like he did then writing all letters by hand. \$\endgroup\$
    – t3chb0t
    Sep 28, 2016 at 10:45
  • \$\begingroup\$ Yes, easier. But now every time EnglishAlphabet is accessed, this loop is going to be ran. The readonly static puts it directly into memory with basically no computation. Additionally, if the OP wants to say only check for vowels then this version would be easier to modify. \$\endgroup\$
    – JD.B
    Sep 28, 2016 at 12:27
  • \$\begingroup\$ @JD.B with basically no computation because it's soooo important here ;-] if the OP wants to say only check for vowels why would he want to do it? he's checking strings for being panagrams and not containing vowels :-\ \$\endgroup\$
    – t3chb0t
    Sep 28, 2016 at 13:29
4
\$\begingroup\$

I would suggest that you consider using an immutable set:

static readonly ImmutableHashSet<char> alphabet = 
  ImmutableHashSet<char>.Empty.Union("abcdefghijklmnopqrstuvwxyz");
static bool IsPangram(string s)
{
  return alphabet.Intersect(s.ToLower()).Count == alphabet.Count;
}

Keep it short and simple; use off-the-shelf parts.

\$\endgroup\$
1
  • \$\begingroup\$ well, this I like ;-) \$\endgroup\$
    – t3chb0t
    Sep 28, 2016 at 15:36
2
\$\begingroup\$

As others have pointed out you only have to check the number of distinct characters in the sentence. A one liner doing that using linq could be:

public static bool IsPangram2(this string text, int alphabetLength = 26)
{
  return text.ToUpper().Where(ch => char.IsLetter(ch)).Distinct().Count() >= alphabetLength;
}

As stated in a comment the above solution does not handle accented letters so a better solution may be:

private static string DefaultCharacters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

public static bool IsPangram3(this string text, string validCharacters = null)
{
  text = text.ToUpper();
  validCharacters = validCharacters?.ToUpper() ?? DefaultCharacters;
  return !validCharacters.Any(ch => !text.Contains(ch));
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ This doesn't work with any kind of accented characters in the string "abcdeèéhijklmnopqrstuvwxyz".IsPangram2() -> true. Using ToUpper and ToLower for case insensitive comparisons is a bad practice. \$\endgroup\$
    – Johnbot
    Sep 28, 2016 at 8:01
1
\$\begingroup\$

Since you're (apparently) already changing to a version that uses a set (and its size) instead of a dictionary, I think it's also worth considering letting the user just pass the size of the alphabet they care about.

Using this your convenience overload would be just:

public static bool IsPangram(this string source)
{
    return source.IsPangram(26);
}

You really only care about the size of the alphabet anyway, so you might as well just pass the size. In the specific case of the English alphabet, it's quick and easy to generate all the lower-case letters, but for many other alphabets, this is a more difficult task--for example, for French or Spanish lower-case letters, it looks like you need to specify around 8 separate ranges of characters apiece (but it seems safe to assume French and Spanish speakers know the number of letters in their alphabets just as well as English speakers do). In short, passing the number is a lot easier than passing all the elements (which you really don't care about anyway).

\$\endgroup\$
1
\$\begingroup\$

Make it short, make it regex:

public static bool IsPangram(this string input)
{
    const int alphabetLength = 26;
    return Regex.Matches(input, @"[a-z]", RegexOptions.IgnoreCase)
        .Cast<Match>()
        .Select(m => m.Groups[0].Value.ToLower())
        .Distinct()
        .Count() == alphabetLength;
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ You should call Distinct with a case insensitive comparer instead of calling ToLower. \$\endgroup\$
    – Johnbot
    Sep 28, 2016 at 8:04
  • \$\begingroup\$ It also can't be used in case the alphabet is different from the English one. \$\endgroup\$ Sep 28, 2016 at 8:12
  • \$\begingroup\$ @GentianKasa you exaggerate, is it really so hard to adjust the regex for other languages and/or add another parameter? \$\endgroup\$
    – t3chb0t
    Sep 28, 2016 at 10:42
  • \$\begingroup\$ @Johnbot thx for pointing this out ;-) \$\endgroup\$
    – t3chb0t
    Sep 28, 2016 at 10:47
  • \$\begingroup\$ @t3chb0t the difficulty is in adapting the regex. For example, the alphabet can the English one, Russian, Chinese, Arabic, [0-9], [actg], and anything else one can think of. By using a regex it becomes much more difficult to understand which alphabet is being used. \$\endgroup\$ Sep 28, 2016 at 14:32
-2
\$\begingroup\$

Without a line by line review
Just an implementation of what Janos answered
Give the check to Janos

    private HashSet<int> panaHash = new HashSet<int>();
    int cI;
    public bool IsPangram(string source)
    {
        panaHash.Clear();
        foreach (char c in source.ToLower())
        {
            cI = (int)c;
            if (cI < 97)   // not going to change in your life time 
                continue;
            if (cI > 122)  // not going to change in your life time 
                continue;
            panaHash.Add(cI);
            if (panaHash.Count == 26)
                return true;
        }
        return false;
    }

ran some time test and this was the fastest

me 1924
RegX 27436
Lippert 4587
Question 2357

\$\endgroup\$
7
  • \$\begingroup\$ Down votes what is the problem? I don't think you are going to do it faster than this. \$\endgroup\$
    – paparazzo
    Sep 28, 2016 at 12:59
  • \$\begingroup\$ @t3chb0t It is not really the votes I am concerned about. I want to know what is the problem? \$\endgroup\$
    – paparazzo
    Sep 28, 2016 at 16:20
  • \$\begingroup\$ @t3chb0t it is faster than yours by a fact of 10 \$\endgroup\$
    – paparazzo
    Sep 28, 2016 at 16:30
  • \$\begingroup\$ I didn't test it but I believe you ;-) and I didn't DV you. I suggest you write a few more words about your code. Perhaps this is what they don't like. \$\endgroup\$
    – t3chb0t
    Sep 28, 2016 at 16:33
  • \$\begingroup\$ It's nice that you put out an implementation, and benchmark results, it's just that none of this reviews the posted code by OP. As such, I'm afraid this is not an answer on Code Review. Do take a look at our how-to-answer page. \$\endgroup\$
    – janos
    Sep 29, 2016 at 5:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.