patterncsharpMinor
Is the string a pangram?
Viewed 0 times
pangramthestring
Problem
The following code checks if a string is a pangram or not:
Do you see anything wrong/weird in it? Any feedback (style, naming, performance) is more than welcome.
public static class StringExtensions
{
public static IEnumerable EnglishAlphabet
{
get
{
for (char i = 'a'; i
/// Check if the source string is a pangram.
///
///
/// The default dictionary is the english one.
///
public static bool IsPangram(this string source)
{
return source.IsPangram(EnglishAlphabet);
}
///
/// Check if the source string is a pangram.
///
///
/// A pangram is a string that uses every letter in the alphabet.
///
public static bool IsPangram(this string source, IEnumerable alphabet)
{
var lowerAlphabet = new string(alphabet.ToArray()).ToLower();
IDictionary 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.
Solution
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 (
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
Generate the alphabet once
The English alphabet is regenerated on each call of
This is unnecessary, as it never changes between calls.
You could generate it once and reuse.
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.
Context
StackExchange Code Review Q#142650, answer score: 9
Revisions (0)
No revisions yet.