Mdthbs

I'm practicing for my Technical Interview and I'm worried my code does not meet the bar. What can I do to improve this answer?

Here is the question: https://www.careercup.com/question?id=5739126414901248

Given

colors = ["red", "blue", "green", "yellow"]; and a string

str = "Lorem ipsum dolor sit amet";

write a function that prints each letter of the string in different colors. ex. L is red, o is blue, r is green, e is yellow, m is red, after the space, i should be blue.

My answer:

static void TestPrintColors()

    {

        string[] colors = new string[4] {"red", "blue", "green", "yellow"};

        string str = "Lorem ipsum dolor sit amet";

        PrintColors(colors, str);

    }



    static void PrintColors(string[] colors, string str)

    {

        char log;

        ConsoleColor originalColor = Console.ForegroundColor;

        int colorIndex = 0;

        ConsoleColor currentColor = originalColor;

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

        {

            log = str[i];



            if (log == ' ')

            {

                Console.WriteLine(log);

                continue;

            }



            switch(colors[colorIndex])

            {

                case "red":

                currentColor = ConsoleColor.Red;

                break;



                case "blue":

                currentColor = ConsoleColor.Blue;

                break;



                case "green":

                currentColor = ConsoleColor.Green;

                break;



                case "yellow":

                currentColor = ConsoleColor.Yellow;

                break;



                default:

                currentColor = originalColor;

                break;

            }



            colorIndex++;



            if (colorIndex >= colors.Length)

            {

                colorIndex = 0;

            }



            Console.ForegroundColor = currentColor;



            Console.WriteLine(log);

        }



        Console.ForegroundColor = originalColor;

    }

Don't worry about performance.

You're writing a handful of strings to the console, and it takes milliseconds. When you start writing tens of thousands of strings, and notice it taking seconds, then you can you start to look for optimizations. Until then, the cleanliness of your code is much more important.

Your function is too long.

This is a fifty-line function. It has the responsibilities of iterating through two lists in parallel, skipping spaces, parsing color names, setting the console color, writing to console, and resetting the console color when it's all done. That's a lot! Break it up into smaller functions.

Switch statements are ugly.

I don't mean that they are never appropriate, but ConsoleColor is an enum, and it's possible to parse enums (while ignoring case sensitivity). You should replace this switch statement with a function call.

Don't initialize variables until you need them.

With few exceptions, modern languages are very good about optimizing variable allocation. Putting char log = str[i] inside the loop will not result in additional memory usage, and it will save me (a potential reviewer or maintenance programmer) from having to think about that character before or after the loop.

Other tips...

You say this is practice for an interview, so it could be a good place for you to show off your knowledge of C#. With a little trouble, you could leverage Regular Expressions and LINQ to save you from manually manipulating array indexes. With a little more trouble, you could leverage IDisposable to ensure the original ForegroundColor is restored when all is said and done.

On the other hand, you could also shoot yourself in the foot attempting to do those things. If you don't honestly have in-depth knowledge about C#, it might be best just to aim for code that is as simple as possible. I think the best way to do that is to make small functions with clear names, to show you are thinking about the maintainability and reusability of your code.

Declaring array size

Since you're declaring and initializing the array in one step, you don't need to declare the array size. Omitting the array size means you can add/remove elements from the initialization without needing to worry about also changing the number in the declaration.

// before

string[] colors = new string[4] {"red", "blue", "green", "yellow"};





// after

string[] colors = new string[] {"red", "blue", "green", "yellow"};

Circular looping

You can make this a neat one-liner using the modulus operator (or if you want your code to be ugly, a ternary operator). It's personal preference but the modulus operator has the added benefit of allowing you to iterate backwards because it will wrap your index/iterator even if it goes negative.

// before

colorIndex++;



if (colorIndex >= colors.Length)

{

    colorIndex = 0;

}





// after

colorIndex = (colorIndex + 1) % colors.Length;

For loop

You use a for loop to iterate your string, but the iterator/index variable i is only used to retrieve the char element from the appropriate position. Since the rest of your code isn't dependent on the numerical index value, you can just use a foreach loop. Remember to remove the declaration for the log variable if you declare it in your foreach loop.

// before

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

{

    log = str[i];





// after

foreach (char log in str) 

{

Checking for white space

C#'s char primitive type has a built-in check for white space which covers more than just spacebar.

// before

if (log == ' ')



// after

if (char.IsWhiteSpace(log))

Switch statements

As mentioned by another answer, switch statements are ugly. But there's another reason to opt for a different solution: if you want to add more colors to your array, you will also need to remember to add more cases to the switch statement.

// before

switch (colors[colorIndex])

{

    case "red":

        currentColor = ConsoleColor.Red;

        break;



    case "blue":

        currentColor = ConsoleColor.Blue;

        break;



    case "green":

        currentColor = ConsoleColor.Green;

        break;



    case "yellow":

        currentColor = ConsoleColor.Yellow;

        break;



    default:

        currentColor = originalColor;

        break;

}



// after

if (!Enum.TryParse(colors[colorIndex], true, out currentColor))

{

    currentColor = originalColor;

}

The adjustments I pointed out result in significantly less code that is more maintainable. It is functionally the same, and flexible in that you can easily add more colors later if you wanted to by just adding them to your array initialization.