Color-coded console output

Question

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;
 }
}

• RubberDuck: I think you might be interested in this project github.com/DITLeonel/GetBootstrap
• RubberDuck: and basically all of this guy's questions codereview.stackexchange.com/users/29034/leonel?tab=questions
• Malachi: Didn't your mother ever tell you to use Curly Braces?
• BenVlodgi: @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.
• BenVlodgi: @YvesSchelpe yes, but I haven't been maintaining it very well. It is here on GitHub.

Answer 1

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.

• BenVlodgi: 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.
• BenVlodgi: 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.

Answer 2

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.

• BenVlodgi: I abhor curly braces. But I expected nothing less.
• Millie Smith: 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.
• BenVlodgi: @MillieSmith and until I have more than one line of code, I will not add them
• Malachi: 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...