Cfrtjryk

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 the stringToEncode.


• Names of internal variables could be better:
  
  
  
  
  • 
    Convert sounds like a method name. The dictionary should be named Conversions.
  
  
  • 
    nums should be digits
    
  
  
  • 
    sb should be called encoded
    
  
  
  
  


• 
  TryGetValue could have been used instead of ContainsKey
  


• 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 own IEqualityComparer<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.

Additional to @t3chb0t real code review, I would like to provide an alternative more object oriented implementaion.

Not sure if that solution is really better / more readable / appropriate for such a small problem - but at least it follows the SOLID principles:

public class InputProcessor

{

    private static readonly ICharStreamProcessor CharStreamProcessors;



    static  InputProcessor()

    {

        CharStreamProcessors = new ICharStreamProcessor

        {

            new VowelCharStreamProcessor(),

            new YCharStreamProcessor(),

            new WhiteSpaceCharStreamProcessor(),

            new ConsonantCharStreamProcessor(),

            new NumberCharStreamProcessor(),

            new DefaultCharStreamProcessor(),

        };

    }



    public string Process(string input)

    {

        var preprocessedInput = (input ?? string.Empty).ToLower();

        var resultBuilder = new StringBuilder();



        using (var enumerator = preprocessedInput.GetEnumerator())

        {

            var continueProcessing = enumerator.MoveNext();

            while (continueProcessing)

            {

                var processor = CharStreamProcessors.FirstOrDefault(p => p.CanProcess(enumerator.Current));



                if (processor == null)

                {

                    throw new InvalidOperationException($"Unable to find appropriate processor for character '{enumerator.Current}'.");

                }



                bool continueProcessingWithCurrentProcessor;

                ProcessResult result = null;

                do

                {

                    var accumulated = result?.Result ?? string.Empty;

                    result = processor.Process(enumerator.Current, accumulated);

                    continueProcessing = enumerator.MoveNext();



                    continueProcessingWithCurrentProcessor =

                        continueProcessing

                        && result.ContinueConsuming

                        && processor.CanProcess(enumerator.Current);

                } while (continueProcessingWithCurrentProcessor);



                resultBuilder.Append(result.Result);

            }

        }



        return resultBuilder.ToString();

    }

}



public class VowelCharStreamProcessor : ICharStreamProcessor

{

    private static readonly string vowels = "aeiou";

    private const int AsciiNumberFor1 = 49;



    public bool CanProcess(char c) => vowels.Contains(c);

    public ProcessResult Process(char c, string accumulate) => ProcessResult.Finished((char)(vowels.IndexOf(c) + AsciiNumberFor1));

}



public class ConsonantCharStreamProcessor : ICharStreamProcessor

{

    private static readonly string consonants = "bcdefghjklmnpqrstvwxyz";



    public bool CanProcess(char c) => consonants.Contains(c);

    public ProcessResult Process(char c, string accumulate) => ProcessResult.Finished((char)(c - 1));

}



public class YCharStreamProcessor : ICharStreamProcessor

{

    public bool CanProcess(char c) => c == 'y';

    public ProcessResult Process(char c, string accumulate) => ProcessResult.Finished(' ');

}



public class WhiteSpaceCharStreamProcessor : ICharStreamProcessor

{

    public bool CanProcess(char c) => c == ' ';

    public ProcessResult Process(char c, string accumulate) => ProcessResult.Finished('y');

}



public class NumberCharStreamProcessor : ICharStreamProcessor

{

    public bool CanProcess(char c) => char.IsDigit(c);

    public ProcessResult Process(char c, string accumulate) => ProcessResult.Continue(c + accumulate);

}



public class DefaultCharStreamProcessor : ICharStreamProcessor

{

    public bool CanProcess(char c) => true;

    public ProcessResult Process(char c, string accumulate) => ProcessResult.Finished(c);

}



public interface ICharStreamProcessor

{

    ProcessResult Process(Char c, string accumulate);

    bool CanProcess(Char c);

}



public class ProcessResult

{

    private ProcessResult(string result, bool continueConsuming = false)

    {

        this.ContinueConsuming = continueConsuming;

        this.Result = result;

    }



    public bool ContinueConsuming { get; }

    public string Result { get; }



    public static ProcessResult Continue(string result) => new ProcessResult(result, true);

    public static ProcessResult Finished(string result) => new ProcessResult(result);

    public static ProcessResult Finished(char result) => new ProcessResult(result.ToString());

}

You should check the input for a valid string:

  if (string.IsNullOrWhiteSpace(stringToEncode))

    return stringToEncode;

The check for Convert.Count == 0 is a little strange in that Convert is a private field and only set in one method where we can assume it will be constructed equally every time it is called:

if (Convert == null || Convert.Count == 0) BuildConversionMappings();

But no real harm done, without that check you could just simplify the call like this:

Convert = Convert ?? BuildConversionMappings();

You can set the capacity of the StringBuilder which is a performance improvement:

  StringBuilder sb = new StringBuilder(stringToEncode.Length);

You use continue a couple of times in your loop:

    if (Convert.ContainsKey(c))

    {

      sb.Append(Convert[c]);

      continue;

    }



    if (char.IsDigit(c))

    {

      ...

If you replace continue with else if statements it will IMO give a more readable code:

  for (int i = 0; i < stringToEncode.Length; i++)

  {



    char c = stringToEncode[i];

    if (Convert.ContainsKey(c))

    {

      sb.Append(Convert[c]);

    }

    else if (char.IsDigit(c))

    {

      nums.Push(c);

      if (i == stringToEncode.Length - 1

         || !char.IsDigit(stringToEncode[i + 1]))

      {

        while (nums.Count > 0)

        {

          sb.Append(nums.Pop());

        }

      }

    }

    else

    {

      sb.Append(c);

    }

  }

That said, I think you main method and main loop are quite good and easy to read and understand, and the Dictionary idea is just the way to go. But the way you build the dictionary is maybe a little cumbersome.

To the bone you just map between two sets of chars (except for the numbers):

  const string keys =    "abcdefghijklmnopqrstuvwxyz ";

  const string values =  "1abc2efg3ijklm4opqrs5uvw yy";

You could look up by finding the index of chars from the input string in keys and fetch the corresponding value from values (lower case). But that would require two look ups per char, and therefore the Dictionary is much better. A Dictionary<char, char> can be build from the above keys and values in the following way:

private static Dictionary<char, char> ConstructMap()

{

  const string keys =    "abcdefghijklmnopqrstuvwxyz ";

  const string values =  "1abc2efg3ijklm4opqrs5uvw yy";



  IEnumerable<(char c, char cm)> map = keys.Zip(values, (c, cm) => (c, cm));

  return map.ToDictionary(ccm => ccm.c, ccm => ccm.cm);

}

because string implements IEnumerable<char>

Just for the exercise I implemented a quite old school indexed for-loop like this:

private static readonly Dictionary<char, char> charMap = ConstructMap();



public static string HHEncode(string data)

{

  char result = new char[data.Length];



  for (int i = 0; i < data.Length; i++)

  {

    if (charMap.TryGetValue(char.ToLower(data[i]), out char value))

    {

      result[i] = value;

    }

    else if (char.IsDigit(data[i]))

    {

      int j = i + 1;

      while (j < data.Length && char.IsDigit(data[j])) j++;



      j--;

      for (int k = 0; k <= (j - i) / 2; k++)

      {

        result[i + k] = data[j - k];

        result[j - k] = data[i + k];

      }

      i = j;

    }

    else

    {

      result[i] = data[i];

    }

  }



  return new string(result);

}