There are couple of things that might have been done better...
- Initilize the dictionary form a static constructor or call the method to initialize the field. The method should not access it internally but return a dictionary as a result. The field itself should be
readonly
. foreach
could be used to iterate thestringToEncode
.- Names of internal variables could be better:
Convert
sounds like a method name. The dictionary should be namedConversions
.nums
should bedigits
sb
should be calledencoded
TryGetValue
could have been used instead ofContainsKey
- The dictionary is case-insensitive only because you put all letters there. Instead you should use the
StringComparer.OrdinalIgnoreCase
and make the dictionary<string, char>
but this would be inconvenient. You could create your ownIEqualityComparer<char>
. This will cut in half its size . - I wish more
var
(optional) - I wish more LINQ
Things that are already good:
- I find using a dictionary was a good choice.
- The use of a
Stack
to reverse the order is very clever. - Using
StringBuilder
for efficiency is definitely a good choice either.
When we apply all suggestions the code could look like this:
Creating the IReadOnlyDictionary
which returns a result and uses a couple of additional helper variables. No for
loops.
// pre processed conversions for letters
private static readonly IReadOnlyDictionary<char, char> Conversions = BuildConversionDictionary();
private static IReadOnlyDictionary<char, char> BuildConversionDictionary()
{
var conversions = new Dictionary<char, char>(CaseInsensitiveCharComparer);
var alphabet = Enumerable.Range('a', 'z' - 'a' + 1).Select(x => (char)x);
var vowels = new char[] { 'a', 'e', 'i', 'o', 'u' };
var consonants = alphabet.Except(vowels);
// consonants are replaced with previous letter b->a, c->b, etc
foreach (var c in consonants)
{
conversions.Add(c, (char)(c - 1));
}
// y goes to space
// space goes to y
conversions['y'] = ' ';
conversions[' '] = 'y';
// vowels are replaced with number: a->1, e->2, etc
foreach (var (c, i) in vowels.Select((c, i) => (c, i + 1)))
{
conversions.Add(c, (char)('0' + i));
}
return conversions;
}
The alternative equality comparer (here I'm using a helper-factory from my libraries):
private static IEqualityComparer<char> CaseInsensitiveCharComparer =
EqualityComparerFactory<char>.Create
(
(x, y) => StringComparer.OrdinalIgnoreCase.Equals(x.ToString(), y.ToString()),
obj => StringComparer.OrdinalIgnoreCase.GetHashCode(obj.ToString())
);
The refactored encode
(which should be Encode
). What has changed here is that I first check for digits. If it's not one and we have some then dump the stack to the builder and clear it, othewise do the rest. Below the loop we have to dump it once again in case there are some digits left at the end.
public static string Encode(string stringToEncode)
{
if (string.IsNullOrEmpty(stringToEncode)) throw new ArgumentException(nameof(stringToEncode));
// our return val (efficient Appends)
var encoded = new StringBuilder();
// used for reversing the numbers
var digits = new Stack<char>();
// iterate the input string
foreach (var c in stringToEncode)
{
if (char.IsDigit(c))
{
digits.Push(c);
continue;
}
if (digits.Any())
{
encoded.Append(digits.ToArray());
digits.Clear();
}
if (Conversions.TryGetValue(c, out var converted))
{
encoded.Append(converted);
continue;
}
// something else, undefined
encoded.Append(c);
}
// "dump" what's left
encoded.Append(digits.ToArray());
return encoded.ToString();
}
I think you cannot do anything else having only 60 minutes.