patterncsharpMinor
Color-coded console output
Viewed 0 times
codedconsolecoloroutput
Problem
I wrote this
Example syntax would be:
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;
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.
Where's your
It's a little bit unusual to give your class the same name as one in
Don't forget to make it
I had initially thought that it might make sense to construct this
It's inefficient to use
This seems a bit inefficient since you're doing the same matching twice. I strongly suspect that it would be better to skip the
As another reviewer suggested, this is suboptimal. If
This is clever, but it really would be easier and clearer to just use capturing groups in your
This code would be a lot clearer if you used meaningful variable names instead of
You like terseness -- how about the ternary operator here?
Again, very clever, but I think it would be more efficient to just build a
Like the other reviewer said, this
How about
```
if (clearRestOfLine)
ClearRestOfLine();
}
public static void ClearRestOfLine()
public static class ConsoleWhere'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.