8
\$\begingroup\$

I wrote this Console.Write function to use in my applications where I need to easily color-code output.

Example syntax would be:

Console.WriteLine("<f=red>this is in red");
Console.WriteLine("<b=gray><f=white>This is white on gray. <f=green>This is green on gray. <b=darkmagenta>This is green on dark magenta. <b=d>This is green on default.");

Which results in this...

Console output of code

I'm looking for a general review of the code.

//github.com/BenVlodgi/CommandHelper
public static class Console
{
    private static Regex _writeRegex = new Regex("<[fb]=\\w+>");

    public static void WriteLine(string value, int? cursorPosition = null, bool clearRestOfLine = false)
    {
        Write(value + Environment.NewLine, cursorPosition, clearRestOfLine);
    }

    public static void Write(string value, int? cursorPosition = null, bool clearRestOfLine = false)
    {
        if (cursorPosition.HasValue)
            System.Console.CursorLeft = cursorPosition.Value;

        ConsoleColor defaultForegroundColor = System.Console.ForegroundColor;
        ConsoleColor defaultBackgroundColor = System.Console.BackgroundColor;

        var segments = _writeRegex.Split(value);
        var colors = _writeRegex.Matches(value);

        for (int i = 0; i < segments.Length; i++)
        {
            if (i > 0)
            {
                ConsoleColor consoleColor;
                // Now that we have the color tag, split it int two parts, 
                // the target(foreground/background) and the color.
                var splits = colors[i - 1].Value
                    .Trim(new char[] { '<', '>' })
                    .Split('=')
                    .Select(str => str.ToLower().Trim())
                    .ToArray();

                // if the color is set to d (default), then depending on our target,
                // set the color to be the default for that target.
                if (splits[1] == "d")
                    if (splits[0][0] == 'b')
                        consoleColor = defaultBackgroundColor;
                    else
                        consoleColor = defaultForegroundColor;
                else
                    // Grab the console color that matches the name passed. 
                    // If none match, then return default (black).
                    consoleColor = Enum.GetValues(typeof(ConsoleColor))
                        .Cast<ConsoleColor>()
                        .FirstOrDefault(en => en.ToString().ToLower() == splits[1]);

                // Set the now chosen color to the specified target.
                if (splits[0][0] == 'b')
                    System.Console.BackgroundColor = consoleColor;
                else
                    System.Console.ForegroundColor = consoleColor;
            }

            // Only bother writing out, if we have something to write.
            if (segments[i].Length > 0)
                System.Console.Write(segments[i]);
        }

        System.Console.ForegroundColor = defaultForegroundColor;
        System.Console.BackgroundColor = defaultBackgroundColor;

        if (clearRestOfLine)
            ClearRestOfLine();
    }

    public static void ClearRestOfLine()
    {
        int winTop = System.Console.WindowTop;
        int left = System.Console.CursorLeft;
        System.Console.Write(new string(' ', System.Console.WindowWidth - left));
        System.Console.CursorLeft = left;
        System.Console.CursorTop--;
        System.Console.WindowTop = winTop;
    }
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I think you might be interested in this project github.com/DITLeonel/GetBootstrap \$\endgroup\$ – RubberDuck Apr 15 '15 at 18:56
  • 1
    \$\begingroup\$ and basically all of this guy's questions codereview.stackexchange.com/users/29034/leonel?tab=questions \$\endgroup\$ – RubberDuck Apr 15 '15 at 18:57
  • 2
    \$\begingroup\$ Didn't your mother ever tell you to use Curly Braces? \$\endgroup\$ – Malachi Apr 15 '15 at 19:08
  • 2
    \$\begingroup\$ @RubberDuck Boot strap is neat, but I've written my own methods like those before, and they still have the problem being separate calls for each color desired. I wrote this so that I could inline the changing of colors. \$\endgroup\$ – BenVlodgi Apr 15 '15 at 19:32
  • 1
    \$\begingroup\$ @YvesSchelpe yes, but I haven't been maintaining it very well. It is here on GitHub. \$\endgroup\$ – BenVlodgi Jan 25 '16 at 20:02
6
\$\begingroup\$

Comments are inline.

public static class Console

Where's your using directives? They're needed for the code to compile; you should post them.

It's a little bit unusual to give your class the same name as one in System but I see why you did it -- so you can just switch to using your new module with a using.

{
    private static Regex _writeRegex = new Regex("<[fb]=\\w+>");

Don't forget to make it readonly. The pattern for that Regex should be specified as a verbatim string literal so you don't have to double the backslashes (only you can prevent Leaning Toothpick Syndrome) -- @"<[FB]\w+>". You don't save length but it's easier to understand.

I had initially thought that it might make sense to construct this Regex using RegexOptions.Compiled, but it turns out that has no benefit since it's only used in a static method, so it will automatically be cached. But if you ever use nonstatic methods, it's worth trying it and see if it gives a speed increase.

    public static void WriteLine(string value, int? cursorPosition = null, bool clearRestOfLine = false)
    {
        Write(value + Environment.NewLine, cursorPosition, clearRestOfLine);
    }

It's inefficient to use + here to append onto the string. Since .NET System.String is immutable, it has to build a whole new string and copy. It would be better to just call Write with the parameters as you got them, followed by System.Console.Write(Environment.NewLine);.

    public static void Write(string value, int? cursorPosition = null, bool clearRestOfLine = false)
    {
        if (cursorPosition.HasValue)
            System.Console.CursorLeft = cursorPosition.Value;

        ConsoleColor defaultForegroundColor = System.Console.ForegroundColor;
        ConsoleColor defaultBackgroundColor = System.Console.BackgroundColor;

        var segments = _writeRegex.Split(value);
        var colors = _writeRegex.Matches(value);

This seems a bit inefficient since you're doing the same matching twice. I strongly suspect that it would be better to skip the .Split call and use a foreach loop on the Matches.

        for (int i = 0; i < segments.Length; i++)
        {
            if (i > 0)

As another reviewer suggested, this is suboptimal. If i==0 is a special case, move it outside the loop so you don't need to test i each time through the loop. (The compiler will probably do this for you, so you won't gain any speed -- it's just for code legibility)

            {
                ConsoleColor consoleColor;
                // Now that we have the color tag, split it int two parts, 
                // the target(foreground/background) and the color.
                var splits = colors[i - 1].Value
                    .Trim(new char[] { '<', '>' })
                    .Split('=')
                    .Select(str => str.ToLower().Trim())
                    .ToArray();

This is clever, but it really would be easier and clearer to just use capturing groups in your Regex so you can access the two parts directly from the Match.

                // if the color is set to d (default), then depending on our target,
                // set the color to be the default for that target.
                if (splits[1] == "d")
                    if (splits[0][0] == 'b')

This code would be a lot clearer if you used meaningful variable names instead of splits[1] and splits[0].

                        consoleColor = defaultBackgroundColor;
                    else
                        consoleColor = defaultForegroundColor;

You like terseness -- how about the ternary operator here?

                else
                    // Grab the console color that matches the name passed. 
                    // If none match, then return default (black).
                    consoleColor = Enum.GetValues(typeof(ConsoleColor))
                        .Cast<ConsoleColor>()
                        .FirstOrDefault(en => en.ToString().ToLower() == splits[1]);

Again, very clever, but I think it would be more efficient to just build a Dictionary<string,ConsoleColor> one time at the start, rather than searching the whole list each time. LINQ is extremely useful; that doesn't mean it's best used everywhere.

                // Set the now chosen color to the specified target.
                if (splits[0][0] == 'b')
                    System.Console.BackgroundColor = consoleColor;
                else
                    System.Console.ForegroundColor = consoleColor;
            }

            // Only bother writing out, if we have something to write.
            if (segments[i].Length > 0)
                System.Console.Write(segments[i]);
        }

Like the other reviewer said, this if should be at the start so you don't bother unpacking the string at all.

        System.Console.ForegroundColor = defaultForegroundColor;
        System.Console.BackgroundColor = defaultBackgroundColor;

How about System.Console.ResetColor()? It would make these two variables unnecessary.

        if (clearRestOfLine)
            ClearRestOfLine();
    }

    public static void ClearRestOfLine()
    {
        int winTop = System.Console.WindowTop;
        int left = System.Console.CursorLeft;
        System.Console.Write(new string(' ', System.Console.WindowWidth - left));
        System.Console.CursorLeft = left;
        System.Console.CursorTop--;
        System.Console.WindowTop = winTop;
    }
}

Does this do what you want after a WriteLine? It looks to me like it clears the next line not the current one.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks @Snowbody, you brought up some very good points. WriteLine does not work as I intended, though it does, if I take your recommondation to use a separate System.Console.WriteLine(); after 'Write`ing out the text. \$\endgroup\$ – BenVlodgi Apr 16 '15 at 15:02
  • \$\begingroup\$ I keep the currently set console colors as the defaults, because they may be different from the actual console defaults. On top of that, I need to be able to just set one at a time and not both. \$\endgroup\$ – BenVlodgi Apr 16 '15 at 15:55
5
\$\begingroup\$

you should definitely be using Curly Braces here

            // if the color is set to d (default), then depending on our target,
            // set the color to be the default for that target.
            if (splits[1] == "d")
                if (splits[0][0] == 'b')
                    consoleColor = defaultBackgroundColor;
                else
                    consoleColor = defaultForegroundColor;
            else
                // Grab the console color that matches the name passed. 
                // If none match, then return default (black).
                consoleColor = Enum.GetValues(typeof(ConsoleColor))
                    .Cast<ConsoleColor>()
                    .FirstOrDefault(en => en.ToString().ToLower() == splits[1]);

You have comments and multilined, single line code inside of an else statement. This calls for Curly Braces

and the if/else block inside of another if block, and no Curly Braces?! this is begging for Curly Braces.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ I abhor curly braces. But I expected nothing less. \$\endgroup\$ – BenVlodgi Apr 15 '15 at 19:34
  • 1
    \$\begingroup\$ Abhor them all you want, but there's no getting rid of them when you have more than one line. Which is a lot of code. \$\endgroup\$ – Millie Smith Apr 16 '15 at 4:23
  • \$\begingroup\$ @MillieSmith and until I have more than one line of code, I will not add them \$\endgroup\$ – BenVlodgi Apr 16 '15 at 13:48
  • \$\begingroup\$ an if and else statement counts as more than one line of code inside of another if statement, even if it compiles the way you anticipate, it is still a bad habit to remove the curly braces from such a block of code. but hey maybe you would like VB or Python much better than C# or Java... \$\endgroup\$ – Malachi Apr 16 '15 at 13:58

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.