10
\$\begingroup\$

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;
    }
\$\endgroup\$
  • \$\begingroup\$ Have you tested the code? If you have tested the code could you please provide the code that calls this function? \$\endgroup\$ – pacmaninbw May 16 at 22:40
  • \$\begingroup\$ @pacmaninbw The calling function is TestPrintColors(). That in turn would be called from a Console Application's Main method. \$\endgroup\$ – benj2240 May 17 at 0:02
11
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Nicely said! Thank you for the great response! I would like to update my code and get feedback, what is the best way to do that? Sorry I'm, new here. Thank you! \$\endgroup\$ – Austin Taylor May 16 at 22:04
  • 2
    \$\begingroup\$ Welcome to CodeReview! The usual rule is that you may make edits to your code until the first answer is posted. At this point, if you were to edit this question my answer would look silly. Please, feel free to post a follow-up question with your updated code, maybe linking back to this one for reference. \$\endgroup\$ – benj2240 May 16 at 22:18
  • \$\begingroup\$ Nice answer! Upvoted! Could you implement your comments in code? \$\endgroup\$ – StepUp May 17 at 14:32
  • \$\begingroup\$ Variable allocation in this case isn’t a question of optimisation. The language requires that the variable inside the loop only takes a single stack address, which gets reused. This is pretty much universal. \$\endgroup\$ – Konrad Rudolph May 17 at 14:47
  • 1
    \$\begingroup\$ practice for an interview ... it could be a good place for you to show off your knowledge of C#. "Practice", fine. Especially if you write different versions as a compare and contrast exercise. But I strongly councel caution to consider situations. Wanton cleverness is detrimental to code maintenance and sometimes a job interview fail. Good OO skills are more important. And clever code makes that skill imperative as it exacerbates weak OO structure. \$\endgroup\$ – radarbob May 21 at 19:22
5
\$\begingroup\$

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.

\$\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.