Skip to main content
deleted 1 character in body
Source Link
user73941
user73941

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:

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:

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:

Source Link
user73941
user73941

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);
}

The main difference to yours is how I handle numbers