String reversal, capitalize vowels, lowercase consonants

Question

This was a portion of a skills test given to me by a company. The task was to write a method that takes a string, reverses it, and sets the vowels as capitals and consonants to lowercase. I was also told to prove that my implementation works.

using System;
using System.Diagnostics;


namespace StringOperation
{
    using static Console;

    internal class Program
    {
        #region Entry

        private static void Main()
        {
            while (true)
            {
                WriteLine(MessageStrings.REQUEST_NAME);
                var strResult = ReadLine();

                var result = ProcessInput(string.IsNullOrEmpty(strResult) ? MessageStrings.PROOF : strResult );

                if ((ReadKey(true).Key ^ ConsoleKey.Y) == 0)
                    continue;
                break;
            }
        }

        #endregion

        #region Logic

        private static string ProcessInput(string input)
        {
            var output = new char[input.Length];

            var desc = output.Length;
            var asnd = -1;

            while(asnd++ <= desc--)
            {
                // unroll and reverse
                output[asnd] = Shift(input[desc]);
                output[desc] = Shift(input[asnd]);                  
            }

            return new string(output);
        }

        private static char Shift(char target)
        {
            // switching the 32 bit and returning the new value
            var i = (target | 0x20);

            // Checking if a the new value is a symbol
            if (i < 97 || i > 122)
                return target;

            switch (i)
            {

                case 97:
                case 101:
                case 105:
                case 111:
                case 117:
                    // Returning a capital vowel
                    return (char) (target & ~0x20);
                default:
                    // returning Lower case consonant
                    return (char) (i);
            }
        }

        #endregion
    }
}

Answer 1

We have a namespace called StringOperation, a class called Program, and methods called Main, ProcessInput, Shift, regions called Entry, Logic.

Looking from a high level, this program could do anything, as these terms are completely meaningless. Strive to find more meaningful names for your program elements, so that readers can have a clue what the program is about without having to fully read the implementation. If I can see in front of me the logical pieces of the puzzle, the mental burden of understanding what goes on in each element would be much reduced.

I know that all self-respecting programmers know by heart that 97 means the ASCII code of the letter A, 101 the letter E, 105 the letter I, ... Wait a minute, no, actually that's not a requirement, I don't think anybody should have to know at a glance what these numbers mean in your case statements. A better way would be to write like this:

case 'A':
case 'E':
case 'I':
// ...

Answer 2

A skill test IMHO involves not only to write performant code but also

• the ability to write code where coding errors can't easily happen

  You should use braces for single if statements too, this makes your code less error prone.

  See: https://www.imperialviolet.org/2014/02/22/applebug.html

  if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) 
      goto fail;  
  if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) 
      goto fail; 
      goto fail; 
  if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) 
      goto fail;
  

  it also helps to grasp the relationship of the statements at first glance.

  If you prefer to omit the braces you should place the command on the same line.

• the ability to write code which is easily maintainable and understandable

  /// <summary>
  /// This method expects the char to be lower case
  /// </summary>
  /// <param name="c">The lower case char</param>
  /// <returns>A upper case char if the char is a vowel, otherwise it returns the char itself</returns>
  private static char SwitchVowelToUpperCase(char c)
  {
      switch (c)
      {
          case 'a': return 'A';
          case 'e': return 'E';
          case 'i': return 'I';
          case 'o': return 'O';
          case 'u': return 'U';
          default: return c;
      }
  }  
  

  here you can see at first glance what the method is doing, without any fancy | and ~ (which maybe is faster).

• the ability to not overcomplicate tasks (speed isn't always the goal)

  Together with the method above this would be sufficient

  private static string ProcessInput(string input)
  {
      var resultingChars = input.ToLower()
                                .Reverse()
                                .Select(c => SwitchVowelToUpperCase(c))
                                .ToArray();
  
      return new string(resultingChars);
  }
  

  Edit

  But we still can do better than that. We know, that we only need to convert the vowel to uppercase and keep the remaining like it is. Let as assume we should process a string like "tEst" then the vowel already is upper case. By using the above ProcessInput() method we would first change the whole string to lowercase and then check if it is a lowercase vowel.
  By omitting the call to ToLower() we get the same result but gain some performance. What we have to do additional is to rename the former SwitchVowelToUpperCase() method to SwitchLowerVowelToUpperCase() to make the intention of the method more clear.

  private static string ProcessInput(string input)
  {
      var resultingChars = input.Reverse()
                                .Select(c => SwitchLowerVowelToUpperCase(c))
                                .ToArray();
  
      return new string(resultingChars);
  }
  

  ---

Regions

Please read are-regions-an-antipattern-or-code-smell

Is there a good use for regions?

No. There was a legacy use: generated code. Still, code generation tools just have to use partial classes instead. If C# has regions support, it's mostly because this legacy use, and because now that too many people used regions in their code, it would be impossible to remove them without breaking existent codebases.

Think about it as about goto. The fact that the language or the IDE supports a feature doesn't mean that it should be used daily. StyleCop SA1124 rule is clear: you should not use regions. Never.

---

Answer 3

I can think of two ways to make your code clearer. First, use String.ToLower():

// Assume more consonants than Vowels.
string lowerInput = input.toLower();

A library function is likely to be faster than something you code as well as easier to read.

Second, in your switch statement:

switch (i)
{
  // Return a capital vowel.
  case 'a': return 'A';
  case 'e': return 'E';
  case 'i': return 'I';
  case 'o': return 'O';
  case 'u': return 'U';

  // Return lower case consonant.
  default:
    return (char) (i);
}

That takes longer to write, but again makes it clearer what you are doing.

Answer 4

Your code crashes when the length of the string is 0 or 1. That makes it a failure. Since the most important property of code is to be correct, and you made your code incorrect by doing a pointlessly complicated loop and getting it wrong, you failed. I leave it up to you to find the bug.

And it never occured to you that the string could contain anything other than letters, right? Like CR/LF, or any of @[\]^_. Failed again.

BTW. That was one glance at your code, I thought "I bet that's wrong", and half a minute to prove it was wrong. Half a minute instead of 10 seconds because the loop confused me - that's the kind of loop you would write for in-place reversal, where the bug would have hit you in the middle of the string.

BTW. Does that company really not care about Unicode? I find that hard to believe. Did you at least ask about that before you wrote the code?

Answer 5

This code looks a little silly to me, I would do it a little bit differently to make it a little easier to read

    private static string ProcessInput(string input)
    {
        var output = new char[input.Length];

        var desc = output.Length;
        var asnd = -1;

        while(asnd++ <= desc--)
        {
            // unroll and reverse
            output[asnd] = Shift(input[desc]);
            output[desc] = Shift(input[asnd]);                  
        }

        return new string(output);
    }

I would have done it with a for loop and declared the variables in the loop initialization, this is why they made for loops, your while loop had too much going on in it's condition statement.

Here is how I would have written it

private static string ProcessInput(string input)
{
    var output = new char[input.Length];

    for (int desc = output.Length, asnd = -1; asnd <= desc; asnd++, desc--)
    {
        // unroll and reverse
        output[asnd] = Shift(input[desc]);
        output[desc] = Shift(input[asnd]);
    }
    return new string(output);
}

This doesn't fix the issue of having a single character string or an odd count string and it actually brings a couple more issues with it.

1. It will give us an index out of range error
2. It performs shifts more than once
3. There really isn't a reason for the second line of code inside the loop

I would drop that second line of code and fix the initialization of the loop like this

private static string ProcessInput(string input)
{
    var output = new char[input.Length];
    for (int desc = output.Length - 1, asnd = 0; asnd < output.Length; asnd++, desc--)
    {
        // unroll and reverse
        output[asnd] = Shift(input[desc]);
    }
    return new string(output);
}

Answer 6

What's the reason for doing two elements at once:

        while(asnd++ <= desc--)
        {
            // unroll and reverse
            output[asnd] = Shift(input[desc]);
            output[desc] = Shift(input[asnd]);                  
        }

This is actually less efficient than processing characters sequentially one at a time, due to caching issues. The code as written breaks locality: for a long string it looks at areas of memory that are far apart, halving the effectiveness of the caches.

Also, in the case of an odd-length string, it does the work (array-retrieve, Shift(), and array-store) twice for the middle element.

Answer 7

You can make use of char static methods to do a lot of the work for you. Also, there's no need to memorize the ASCII values of vowels. Just use the vowels themselves.

static string Problem(string arg)
{
    if (arg == null)
        return null;

    char[] vowels = "aeiouAEIOU".ToCharArray();
    StringBuilder result = new StringBuilder();

    for (int i = arg.Length - 1; i >= 0; i--)
    {
        char ch = arg[i];

        if (vowels.Contains(ch))
            result.Append(char.ToUpper(ch));

        else
            result.Append(char.ToLower(ch));
    }

    return result.ToString();
}

This solution relies on the assumption that we're working with English. It's a good idea in interviews to clarify the question so that you don't have to make assumptions. Since this was a test, you may not have had the opportunity to ask questions. In that case, simply state your assumptions in code comments.

Answer 8

Why not using LINQ to get something much more readable ?

    public static string ProcessInput(string str)
    {
        char[] v = { 'a', 'e', 'i', 'o', 'u' };
        char[] result = str.Reverse()
                           .Select(c => v.Contains(char.ToLower(c)) ? char.ToUpper(c) : char.ToLower(c))
                           .ToArray();

        return new String(result);
    }