Etydhv

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.