Console window to debug Windows Applications

Question

Since I was having some difficulties to debug the next step for Scrolly - A (very) simple infinite mouse "scroll", I've decided to make a console-like class that allows to add messages to it.

This allows you to show some messages. It also allows to pass any object you want, but it will be converted to string. You can add information, error and warning messages as well, which will have a matching color for each.

using System;
using System.Drawing;
using System.Windows.Forms;

namespace ConsoleWindow
{
    public sealed partial class Console
    {
        private static volatile Console instance = new Console();
        private static ConsoleForm form = new ConsoleForm();

        public static Console Instance
        {
            get
            {
                return instance;
            }
        }

        public static bool autoShow = true;

        private static string newline = "\r\n";

        private static string line = "================================================================================";

        /// <summary>
        /// Forces the prompt to go to the end of the text.
        /// </summary>
        private static void scrollToEnd()
        {
            form.consoleOutput.SelectionStart = form.consoleOutput.Text.Length;
            form.consoleOutput.ScrollToCaret();
        }

        /// <summary>
        /// Adds a message to the console.
        /// </summary>
        /// <param name="message">Message object to log.</param>
        public static bool Log<T>(T message)
        {
            if (autoShow)
            {
                Show();
            }

            scrollToEnd();
            form.consoleOutput.AppendText(message.ToString() + newline);
            scrollToEnd();

            return true;
        }

        /// <summary>
        /// Adds an error message to the console.
        /// </summary>
        /// <param name="message">Message object to log.</param>
        public static bool Error<T>(T message)
        {
            if (autoShow)
            {
                Show();
            }

            scrollToEnd();
            form.consoleOutput.SelectionColor = Color.Red;
            form.consoleOutput.SelectionLength = 0;

            form.consoleOutput.AppendText("(!) " + message.ToString() + newline);

            scrollToEnd();

            form.consoleOutput.SelectionColor = form.consoleOutput.ForeColor;

            return true;
        }

        /// <summary>
        /// Adds a warning message to the console.
        /// </summary>
        /// <param name="message">Message object to log.</param>
        public static bool Warning<T>(T message)
        {
            if (autoShow)
            {
                Show();
            }

            scrollToEnd();
            form.consoleOutput.SelectionColor = Color.Yellow;
            form.consoleOutput.SelectionLength = 0;

            form.consoleOutput.AppendText(@"/!\ " + message.ToString() + newline);

            scrollToEnd();

            form.consoleOutput.SelectionColor = form.consoleOutput.ForeColor;

            return true;
        }

        /// <summary>
        /// Adds a warning error message to the console.
        /// </summary>
        /// <param name="message">Message object to log.</param>
        public static bool Info<T>(T message)
        {
            if (autoShow)
            {
                Show();
            }

            scrollToEnd();
            form.consoleOutput.SelectionColor = Color.LightBlue;
            form.consoleOutput.SelectionLength = 0;

            form.consoleOutput.AppendText("[!] " + message.ToString() + newline);

            scrollToEnd();

            form.consoleOutput.SelectionColor = form.consoleOutput.ForeColor;

            return true;
        }

        /// <summary>
        /// Clears the console text.
        /// </summary>
        public static void Clear()
        {
            form.consoleOutput.Clear();

            bool autoShowOriginal = autoShow;
            autoShow = false;

            autoShow = autoShowOriginal;
        }

        /// <summary>
        /// Clears the console text. Returns false on exception.
        /// </summary>
        /// <param name="filename">Filename path in a string</param>
        /// <param name="log">Logs a message indicating the operation successfulness</param>
        /// <param name="clear">Clears the console on success</param>
        public static bool SaveToFile(string filename, bool log = true, bool clear = false)
        {
            try
            {
                System.IO.File.WriteAllLines(form.saveFile.FileName, form.consoleOutput.Lines);

                if (clear)
                {
                    Clear();
                }
                if (log == true)
                {
                    Log(line);
                    Info("File saved: " + filename);
                }

                return true;
            }
            catch (System.IO.IOException e)
            {
                if (log == true)
                {
                    Log(line);
                    Error("Failed to save: " + filename);
                    Error(e);
                }

                return false;
            }
        }

        /// <summary>
        /// Closes the console.
        /// </summary>
        public static void Hide()
        {
            form.Hide();
        }

        /// <summary>
        /// Closes the console. Warning: output will be lost!
        /// </summary>
        public static void Close()
        {
            form.Hide();

            Clear();
        }

        /// <summary>
        /// Shows the console.
        /// </summary>
        public static void Show()
        {
            form.Show();
        }


        /// <summary>
        /// Private form functions and elements.
        /// </summary>
        private class ConsoleForm : Form
        {
            private static System.ComponentModel.IContainer components = new System.ComponentModel.Container();

            public RichTextBox consoleOutput = new RichTextBox();
            public ContextMenuStrip consoleMenu = new ContextMenuStrip(components);
            public ToolStripMenuItem consoleMenuSave = new ToolStripMenuItem();
            public ToolStripMenuItem consoleMenuClose = new ToolStripMenuItem();
            public SaveFileDialog saveFile = new SaveFileDialog();

            /// <summary>
            /// Clean up any resources being used.
            /// </summary>
            /// <param name="disposing">true if managed resources should be disposed; otherwise, false.</param>
            protected override void Dispose(bool disposing)
            {
                if (disposing && (components != null))
                {
                    components.Dispose();
                }
                this.Dispose(disposing);
            }

            /// <summary>
            /// Required method for Designer support - do not modify
            /// the contents of this method with the code editor.
            /// </summary>
            private void InitializeComponent()
            {
                this.consoleMenu.SuspendLayout();
                this.SuspendLayout();
                // 
                // consoleOutput
                // 
                this.consoleOutput.BackColor = System.Drawing.Color.Black;
                this.consoleOutput.BorderStyle = System.Windows.Forms.BorderStyle.None;
                this.consoleOutput.ContextMenuStrip = this.consoleMenu;
                this.consoleOutput.DetectUrls = false;
                this.consoleOutput.Dock = System.Windows.Forms.DockStyle.Fill;
                this.consoleOutput.Font = new System.Drawing.Font("Consolas", 9.75F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0)));
                this.consoleOutput.ForeColor = System.Drawing.Color.Silver;
                this.consoleOutput.Location = new System.Drawing.Point(0, 0);
                this.consoleOutput.Name = "consoleOutput";
                this.consoleOutput.ReadOnly = true;
                this.consoleOutput.ScrollBars = System.Windows.Forms.RichTextBoxScrollBars.ForcedVertical;
                this.consoleOutput.Size = new System.Drawing.Size(583, 261);
                this.consoleOutput.TabIndex = 0;
                this.consoleOutput.TabStop = false;
                this.consoleOutput.Text = "";
                // 
                // consoleMenu
                // 
                this.consoleMenu.Items.AddRange(new System.Windows.Forms.ToolStripItem[] {
                    this.consoleMenuSave,
                    this.consoleMenuClose
                });
                this.consoleMenu.Name = "consoleMenu";
                this.consoleMenu.ShowImageMargin = false;
                this.consoleMenu.Size = new System.Drawing.Size(107, 48);
                // 
                // consoleMenuSave
                // 
                this.consoleMenuSave.Name = "consoleMenuSave";
                this.consoleMenuSave.Size = new System.Drawing.Size(106, 22);
                this.consoleMenuSave.Text = "&Save to file";
                this.consoleMenuSave.Click += new System.EventHandler(this.consoleMenuSave_Click);
                // 
                // consoleMenuClose
                // 
                this.consoleMenuClose.Name = "consoleMenuClose";
                this.consoleMenuClose.Size = new System.Drawing.Size(106, 22);
                this.consoleMenuClose.Text = "&Close";
                this.consoleMenuClose.Click += new System.EventHandler(this.consoleMenuClose_Click);
                // 
                // saveFile
                // 
                this.saveFile.DefaultExt = "txt";
                this.saveFile.Filter = "Text file (*.txt)|*.txt|All files|*.*";
                this.saveFile.Title = "Save console output";
                // 
                // consoleForm
                // 
                this.AutoScaleDimensions = new System.Drawing.SizeF(6F, 13F);
                this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font;
                this.ClientSize = new System.Drawing.Size(583, 261);
                this.Controls.Add(this.consoleOutput);
                this.FormBorderStyle = System.Windows.Forms.FormBorderStyle.FixedSingle;
                this.MaximizeBox = false;
                this.MinimizeBox = false;
                this.Name = "consoleForm";
                this.ShowIcon = false;
                this.ShowInTaskbar = false;
                this.Text = "Console";
                this.FormClosing += new System.Windows.Forms.FormClosingEventHandler(this.consoleForm_FormClosing);
                this.consoleMenu.ResumeLayout(false);
                this.ResumeLayout(false);

            }

            public ConsoleForm()
            {
                InitializeComponent();
            }

            private void consoleMenuSave_Click(object sender, EventArgs e)
            {
                if (saveFile.ShowDialog() == DialogResult.OK)
                {
                    Console.SaveToFile(saveFile.FileName);
                }
            }

            private void consoleMenuClose_Click(object sender, EventArgs e)
            {
                Console.Close();
            }

            private void consoleForm_FormClosing(object sender, FormClosingEventArgs e)
            {
                if (e.CloseReason != CloseReason.ApplicationExitCall
                    || e.CloseReason != CloseReason.TaskManagerClosing)
                {
                    e.Cancel = true;

                    Console.Close();
                }
            }
        }
    }
}

It is quite a long wall of code, I know. Most of it is just to set the elements' properties. Most of it is copy-paste from the generated code. I've tried to keep it to a minimum.

The whole idea is to just throw this into a file and you are ready to use it. It should show on your first message. It can be easily disabled.

My main concern is in the whole pattern, which is ... bad... really bad! I have a namespace with a partial class with a private class that inherits from the Form class... But, I can't find any better way to keey the form in a separate class and still private.

In terms of readability, what other crimes am I committing?

Answer 1

public sealed partial class Console

Why is the class partial if there's no designer-generated code associated to it? The partial keyword is provided as a convenience, to help organize code by allowing the definition of a class to span multiple files. If there's only 1 file, the partial keyword is useless at best, and confusing at worst.

At first glance, Console is a confusing name to use, because it clashes with the notion of a Console class anybody remotely familiar with a .net console application will have.

private static volatile Console instance = new Console();

Why static? Why volatile? Even if you're going to be accessing the class from multiple threads, the instance reference is statically initialized, it being volatile is only adding useless overhead.

Looking at the interface (as in, public members) of that class gives me an answer about why the field is static - however best practices tell us that if every member of a class are going to be static, then the class itself should be static as well. This is currently valid code:

var console = new ConsoleWindow.Console();

And then the dev would be all WTF'd over there being no instance members to call. By making the class static, you make invalid the calling of a constructor and force users to call the static members - just like the System.Console class does.

It took me quite a while to realize that it's a Singleton implementation you have here. If your class is static as it should, then you don't need it. And if you really want a Singleton, then you should have a private default constructor, so as to prevent other code from creating an instance of your class... but making the class static is a better way of achieving the same thing.

---

private class ConsoleForm : Form

Why nested? Why private? Why isn't it a partial class, with the designer code in a dedicated designer.cs file?

Most of it is just to set the elements' properties. Most of it is copy-paste from the generated code.

I'm sorry, but no. Just... no.

The Boolean logic is broken here:

if (e.CloseReason != CloseReason.ApplicationExitCall
    || e.CloseReason != CloseReason.TaskManagerClosing)

Read it out loud, or invert the logic to see how.

This is wrong:

private void consoleMenuSave_Click(object sender, EventArgs e)
{
    if (saveFile.ShowDialog() == DialogResult.OK)
    {
        Console.SaveToFile(saveFile.FileName);
    }
}

You're accessing the parent Console class via its static members - but you don't need a nested type to be able to do that. Nested or not, the window is tightly coupled with this parent Console class, and what you have here is presenter logic at the view level.

Make the form stand on its own.

But, I can't find any better way to keep the form in a separate class and still private.

You don't need it private. You just need to know that newing up a form won't do anything, because there's no logic in a form... when it's done right.

Rename your Console class to ConsolePresenter, remove all the static-ness (yes, scratch everything I said above in the first part of this answer!) and make that ConsolePresenter take an IConsoleView instance in its constructor.

Then get that nested type out of there, make it a "normal" form class with the designer code where it belongs (in the .designer.cs file), make it implement that IConsoleView interface, and instead of implementing the logic in the form's code, and instead of calling presenter (/Console) methods directly, raise an event that says "hey there, I'm the form - user just clicked that 'Save' button, anything you'd like to do about it?"

You're going to need the Lines, so make an EventArgs class that includes them:

public class ConsoleLinesEventArgs : EventArgs
{
    public ConsoleLinesEventArgs(string[] lines)
    {
        Lines = lines;
    }

    public string[] Lines { get; private set; }
}

Now use it:

private void consoleMenuSave_Click(object sender, EventArgs e)
{
    OnPromptToSave();
}

public event EventHandler<ConsoleLinesEventArgs> PromptToSave;
protected void OnPromptToSave()
{
    var handler = PromptToSave;
    if (handler != null) 
    {
        var args = new ConsoleLinesEventArgs(consoleOutput.Lines);
        handler(this, args); 
    }
}

The PromptToSave event being part of the IConsoleView interface, the presenter can register it:

public Console(IConsoleView view)
{
    view.PromptToSave += view_PromptToSave;
}

private void view_PromptToSave(object sender, ConsoleLinesEventArgs e)
{
    using (var dialog = new SaveFileDialog())
    {
        if (dialog.ShowDialog() == DialogResult.OK)
        {
            SaveToFile(dialog.FileName, e.Lines);
        }
    }
}

Notice the very, very short lifetime of the dialog instance: because SaveFileDialog implements the IDisposable interface, it's best to keep any instance as short-lived as possible, and to wrap the instance in a using block, so as to ensure proper disposal.

Looking at how SaveToFile is implemented...

System.IO.File.WriteAllLines(form.saveFile.FileName, form.consoleOutput.Lines);

Add using System.IO; at the top of the file, and get rid of these verbose namespace qualifiers: it's File.WriteAllLines you want to call. Now, why are you passing a fileName parameter if the method is reading the file name from the SaveFileDialog instance on the view? This method shouldn't even need to know about the view, let alone about its consoleOutput member, which should be private.

By passing the file name from the SaveFileDialog, and the Lines from the ConsoleLinesEventArgs, you have everything you need to write the lines to a file - and you're not responsible for getting the lines in question.

Once you have the view talking to the presenter only via events, and the presenter only accessing the view via its public interface, then you're ready to drop that flaky Singleton and make the members non-static, and have the class implement some IConsolePresenter interface that client code can use as a mockable dependency instead of being tightly coupled to some static helper methods.

The biggest problem with your code, is that you have class A depending on class B, and class B depending on class A. Refactoring into a Model-View-Presenter pattern will untangle that spaghetti.

---

MVP 101

Let's simplify a bit (this post is already long enough as is) - let's say there's only a single "Export" button and a textbox, and the command associated with the button needs the contents of the textbox - the textbox is an implementation detail, just like the button. All the presenter needs to know, is that the button was clicked:

public interface IFooView
{
    event EventHandler<ExportEventArgs> ExportContent;
    DialogResult ShowDialog();
}

The ExportContent event will be raised by the view when the user clicks the button, to signal the presenter that user wants to export the contents of the textbox. The .NET framework provides a mechanism to carry data with events - that's what the ExportEventArgs class does:

public class ExportEventArgs : EventArgs
{
    public ExportEventArgs(string content)
    {
        Content = content;
    }

    public string Content { get; private set; }
}

Then, you'll have your form, implementing that IFooView interface:

public partial class FooView : Form, IFooView
{
    public FooView()
    {
        InitializeComponents();
    }

    public event EventHandler<ExportEventArgs> ExportContent;
    private void OnExportContent()
    {
        var handler = ExportContent;
        if (handler != null)
        {
            var args = new ExportEventArgs(TextBox1.Text);
            handler(this, args);
        }
    }

    private void ExportButton_Click(object sender, EventArgs e)
    {
        OnExportContent();
    }
}

The default constructor is required for the designer to be able to instantiate the form; by running the InitializeComponents method, it knows how to instantiate and initialize all the controls on the form - that code is generated by the designer, and shouldn't be tampered with. You don't even need to ever look at the FooView.designer.cs file, if the code there ever needs to change, you're better off using the designer and letting the designer regenerate the file.

Next, the event declaration:

public event EventDelegateType EventName;

Events are multicast delegates - they can have multiple subscribers. The framework provides EventHandler for parameterless events, and the generic EventHandler<T> for parameterized ones: sticking to these delegate types is the best way to follow the Principle of Least Surprise, because .NET devs will expect event handlers to have a void:(object, EventArgs) signature - but in theory nothing prevents you from declaring an event with a different delegate type, except...

• Delegate types with a non-void return value are better served by an EventArgs parameter that can pass data back to the event originator.
• Delegate types that explicitly define what the parameters are, will instantly break all client code the minute an event needs to be modified to add, remove, or otherwise modify a parameter: using an EventArgs-derived parameter solves this problem, because by sticking to the pattern your client code will always be valid because as long as a method has an (object,EventArgs) signature, it can handle any event because in C#, FooEventArgs in a delegate signature can perfectly be handled by a handler that takes an EventArgs.

So, events can have subscribers - if you try to raise an event that has no subscriber, a NullReferenceException will be thrown. That's why you're seeing this:

    var handler = ExportContent;
    if (handler != null)
    {
        var args = new ExportEventArgs(TextBox1.Text);
        handler(this, args);
    }

handler is a thread-local copy of the event; raising that copy instead of directly raising the event, makes the event-raising thread safe: another thread could go and un-register the last handler between the handler = assignment and the handler != null check, and no exception would occur, because you're raising a thread-local copy of the event. You don't have to do this, but doing it is best-practice, and not doing it is sure to come and bite you in the future.

Notice this is passed as the sender argument: that's also convention and best-practice. If you had a parameterless event, you would pass EventArgs.Empty instead of null - that's also a convention.

So, the only thing a button click does, is tell whoever is interested that the user clicked a button, and provides whatever data is relevant to carry out whatever task ought to be accomplished.

The presenter is where that logic belongs:

public class FooPresenter
{
    private readonly IFooView _view;

    public FooPresenter(IFooView view)
    {
        _view = view;
        _view.ExportContent += View_ExportContent;
    }

    public void Show()
    {
        _view.ShowDialog();
    }

    private void View_ExportContent(object sender, ExportEventArgs e)
    {
        var data = e.Content;
        // do something with data
    }
}

The presenter is responsible for implementing the logic. It can depend on other services, too - say the ExportContent logic required to write to a database; you could implement the SqlCommand logic there, but that would quickly become redundant if you have several commands that need to be implemented: you would abstract it away behind another interface, and inject that object into your constructor:

    private readonly IFooView _view;
    private readonly IFooService _service;

    public FooPresenter(IFooView view, IFooService service)
    {
        _view = view;
        _service = service;

        _view.ExportContent += View_ExportContent;
    }

    private void View_ExportContent(object sender, ExportEventArgs e)
    {
        _service.Save(e.Content);
    }

Voilà!

What do you get?

• A view that doesn't depend on anything, and that doesn't implement any logic - basically, a view that's just that: a presentation-focused class that's strictly responsible for presentation logic.
• A presenter that depends on an abstraction, so you can mock that IFooView dependency and write automated tests that don't actually need to bring up a form.
• A model that carries data between the view and the presenter.
• Higher cohesion, lower coupling, cleaner code, and a happier maintainer :)

Answer 2

private static string newline = "\r\n";

This is very wrong. And particularly with the open sourcing of .NET leading to the likelihood of C# being uses on non-Windows operating systems increasing, we shouldn't depend on a platform-specific newline signature.

The correct approach looks like this:

private static string newline = Environment.NewLine;

And at this point, we're just providing an alias of questionable utility.

---

Look how much duplication we have between Log, Error, Warn, and Info. This is unnecessary.

What we need is a private internal function that handles everything we're duplicating, and then the Log, Error, Warn, and Info methods simply are publicly exposing various means of calling this method.

private static bool ConsolePrint(String message, Color color)
{
    if (autoShow)
    {
        Show();
    }

    scrollToEnd();
    form.consoleOutput.SelectionColor = color;
    form.consoleOutput.SelectionLength = 0;

    form.consoleOutput.AppendText(message + Environment.NewLine);

    scrollToEnd();
    form.consoleOutput.SelectionColor = form.consoleOutput.ForeColor;

    return true;
}

Now we just wrap this for our other methods.

public static bool Log<T>(T message)
{
    return ConsolePrint(message.ToString, form.consoleOutput.ForeColor);
}

public static bool Error<T>(T message)
{
    return ConsolePrint("(!) " + message.ToString, Color.Red);
}

// etc...

Answer 3

I'm not very familiar with WinForms, or how apps like this are designed, so this will be a short review.

Rather than typing out 80 repeated characters in a string, as you did here:

private static string line = "================================================================================";

You can do something like this, assuming you have .NET 4.0:

private static string line = String.Concat(Enumerable.Repeat("=", 80));

You could also just do this as well (Thanks @Mat'sMug):

private static string line = new string('=', 80);

---

This line:

if (log == true)

Can simply be shortened to this:

if (log)

In addition, since you're already using the namespace System, this line:

catch (System.IO.IOException e)

Can be shortened to this:

catch (IO.Exception e)

There are also a few other places where I've noticed that you've used the prefix System. These can be removed.

Answer 4

/// <summary>
/// Closes the console. Warning: output will be lost!
/// </summary>
public static void Close()
{
    form.Hide();

    Clear();
}

You say you are closing the form, but you are really just hiding it and clearing the data. Types that implement Form have a Close() method.

---

Put a single class/enum/interface in each file. This will make it easy to find the various classes because each file has the same name as the type it contains, and each file has a single responsibility.

---

Now for the main review:

This looks very messy and will be extremely hard to control as the project grows larger. It is time to utilize the MVP pattern.

First, create the interface that your view must implement. That would probably include void Close() and DialogResult ShowDialog(), as well as possibly void Hide() and any other custom methods for directly controlling the form.

Next, create your form. All that code to initialize the form goes in the designer file (but you don't put it there manually, use the designer window to create the output).

Now you create your presenter and DI the form with your custom interface type, something like public MyPresenter(IView view). Call ShowDialog() from there and use the result to decide what to do. If you have multiple buttons that can be clicked, you may need to use Show() and fire events. The presenter will handle the all the data that isn't directly tied to the view. If you want to display output, your presenter should have a public method that takes the output, processes it, and makes a (usually single) call to the view to handle it.

For example, this could be a basic outline of your presenter:

private IView _console;    // assigned from the constructor argument
private static bool ConsolePrint(String message, Color color);
public static bool Log<T>(T message);
public static bool Error<T>(T message);

The model is any supporting methods and other objects, such as enums, that you use.

Now you can also write unit tests to make sure everything works (you may want/need to use Moq or another unit testing framework with this).

Answer 5

In terms of readability, what other crimes am I committing?

You're about to commit one. This is about the maximum amount of content you want in one file. If your namespace grows, make sure every big class gets it's own file. It keeps things organized, maintainable and does wonders for keeping your source-control log clean (you are using source-control, right?).

Take a good look at your function naming:

/// <summary>
/// Closes the console.
/// </summary>
public static void Hide()
{
    form.Hide();
}

Your summary states it's closing. The function name says it's hiding. In my English dictionary, those aren't the same. Unless this is some C# jargon I'm not familiar with, this is highly confusing. Hiding insinuates it's pushed to the background while closing insinuates it's terminated.

I also can't help but notice you use the following a lot:

if (autoShow)
{
    Show();
}

Don't repeat yourself. You're literally copying this construct all over your project. You may want to shove such functions in their own include which may or may not be part of your namespace.