5
\$\begingroup\$

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
    }
}
\$\endgroup\$
  • 6
    \$\begingroup\$ I guess that the requirement "prove that it works" means that you should also write unit tests on your implementation \$\endgroup\$ – almaz Jun 9 '15 at 20:42
  • \$\begingroup\$ See this link thedailywtf.com/articles/… where comments explain in detail why the sub task of reversing a string is a close to unsolvable problem if you want to do it correctly. I wouldn't accept anyone who uses numbers here without a good explanation. \$\endgroup\$ – gnasher729 Jun 10 '15 at 11:45
  • 3
    \$\begingroup\$ Does your code pass the Turkey test? \$\endgroup\$ – Rick Davin Jun 10 '15 at 14:00
16
\$\begingroup\$

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':
// ...
\$\endgroup\$
  • \$\begingroup\$ Good point, I absolutely need to be more descriptive. \$\endgroup\$ – Trae Moore Jun 9 '15 at 19:35
  • \$\begingroup\$ Any critiques about implementation? \$\endgroup\$ – Trae Moore Jun 9 '15 at 19:36
11
\$\begingroup\$

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.


\$\endgroup\$
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I I do agree with making it more legible, however I did do performance testing on the solution. over a million iterations of randomly generated strings with lengths > 1028*1028, the results were that my solution was 400% more efficient than using built in libraries. Feel free to run metrics. These suggestions actually make the code more inefficient. By letting the case statement fall through I remove the redundancy of the multiple returns. Since a char is a stuct, it only seems natural to flip a bit verses using library fuctions since I'm not taking culture into consideration. \$\endgroup\$ – Trae Moore Jun 10 '15 at 1:28
  • \$\begingroup\$ @TraeMoore: Seriously, you are not passing the test. Who seriously cares about the speed of this code. It is unreadable. And instead of worrying about lengths > 1028*1028, you should worry about lengths 0 or 1 where your code crashes. \$\endgroup\$ – gnasher729 Jun 10 '15 at 11:52
  • 3
    \$\begingroup\$ A point that just occurred to me: Your question describes this as a "skills test". One of the skills employers are looking for is knowledge and correct use of the standard libraries. Where there is an obvious method already in the standard library, then use it to show your prospective employer that you are conversant with the library methods. \$\endgroup\$ – rossum Jun 10 '15 at 12:19
6
\$\begingroup\$

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?

\$\endgroup\$
  • 8
    \$\begingroup\$ Nevertheless you spotted the bug(s) please check how-to-be-a-nice-reviewer \$\endgroup\$ – Heslacher Jun 10 '15 at 13:31
  • 2
    \$\begingroup\$ I believe everything here could have been a comment, if written with a different tone, and that this doesn't actually review the code any more than saying, it's broken. \$\endgroup\$ – Malachi Jun 10 '15 at 13:57
  • 5
    \$\begingroup\$ Mod Note: there's nothing I can see in this answer that requires moderator intervention (although the comments will likely need cleaning up). It is an answer in that it is a review of the code. \$\endgroup\$ – rolfl Jun 10 '15 at 14:11
  • 4
    \$\begingroup\$ This is an informative answer, as it makes many important points: the bug, the issue with unsupported characters and unicode, and raises questions the OP should practice asking himself in the future. Unfortunately it's also quite rude. It would be a magnitude better if rephrased dispassionately. \$\endgroup\$ – janos Jun 10 '15 at 15:04
5
\$\begingroup\$

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);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Your last code doesn't work, it only processes half the string. You need to change the termination condition to asnd < output.Length \$\endgroup\$ – Snowbody Jun 11 '15 at 21:16
  • \$\begingroup\$ @Snowbody good catch \$\endgroup\$ – Malachi Jun 11 '15 at 22:01
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Look carefully. It's not only for the middle element of an odd-length string, it's much much worse. You probably don't see it because you would never, ever write the code like that. \$\endgroup\$ – gnasher729 Jun 10 '15 at 12:03
  • \$\begingroup\$ I see two pointers that start out at the start and end of the string and move toward each other. How is that "much much worse"? \$\endgroup\$ – Snowbody Jun 11 '15 at 13:48
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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);
    }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.