HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Color-coded console output

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codedconsolecoloroutput

Problem

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("this is in red");
Console.WriteLine("This is white on gray. This is green on gray. This is green on dark magenta. This is green on default.");


Which results in this...

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

```
//github.com/BenVlodgi/CommandHelper
public static class Console
{
private static Regex _writeRegex = new Regex("");

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

Solution

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("");


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) -- @"". 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  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()
                        .FirstOrDefault(en => en.ToString().ToLower() == splits[1]);


Again, very clever, but I think it would be more efficient to just build a Dictionary 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()

Code Snippets

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)

Context

StackExchange Code Review Q#86995, answer score: 6

Revisions (0)

No revisions yet.