Is it correct to use delegates as properties?

Question

I need to create a class with two properties:

1. LogOutput
2. ExceptionOutput

These properties (of type Action) send a message or a exception depending on the target function. This target function is set via properties. Currently, I have the following code:

public class Output
{
    private Action<string> logOutput;
    private Action<Exception, string> exceptionOutput;

    public Action<string> LogOutput 
    { set { this.logOutput = value; } get { return this.logOutput; } }
    public Action<Exception, string> ExceptionOutput 
    { set { this.exceptionOutput = value; } get { return this.exceptionOutput; } }

    public Output() : this(null, null) { }

    public Output(Action<string> logAction, Action<Exception, string> exceptionAction) 
    {
        this.logOutput = logAction;
        this.exceptionOutput = exceptionAction;
    }

    public void WriteLogMessage(string format, params object[] args) 
    {
        if (this.logOutput != null)
        logOutput(string.Format(format, args));
    }

    public void WriteExceptionMessage(Exception ex, string format, params object[] args) 
    {
        if (this.exceptionOutput != null)
        exceptionOutput(ex, string.Format(format, args));
    }
}

And this is my form code:

private void MainForm_Load(object sender, EventArgs e)
{
    Output myOutput = new Output();

    myOutput.ExceptionOutput = this.WriteExceptionMessageToTextBox;
    myOutput.LogOutput = this.WriteLogMessageToTextBox;

    myOutput.WriteLogMessage("this is my log message to text box");
    myOutput.WriteExceptionMessage(new Exception("this is my exception"), 
        "this is my exception message to text box");
}

private void WriteLogMessageToTextBox(string message)
{
    if (this.txtBox.IsDisposed)
    return;

    if (this.InvokeRequired)
    {
        BeginInvoke(new MethodInvoker(delegate() { WriteLogMessageToTextBox(message); }));
    }
    else 
    {
        this.txtBox.AppendText(message + Environment.NewLine);
    }
}

private void WriteExceptionMessageToTextBox(Exception ex, string message)
{
    if (this.txtBox.IsDisposed)
    return;

    if (this.InvokeRequired)
    {
        BeginInvoke(new MethodInvoker(
            delegate() { WriteExceptionMessageToTextBox(ex, message); }));
    }
    else
    {
        string msg = "";
        msg += string.Format("Program:{0}", message);
        msg += string.Format("Message{0}", ex.Message);
        msg += string.Format("StackTrace:{0}", ex.StackTrace);
        msg += string.Format("Source:{0}", ex.Source);
        this.txtBox.AppendText(msg + Environment.NewLine);
    }
}

The thing is I don't know if this is correct (although it's working). If it's not correct, how can I change it? How can I implement it with events?

Answer 1

Yes, I think this is reasonable code. If you want to set more delegates at the same time or unsubscribe a delegate, events would be more appropriate. But if you don't want to do that, delegate properties are fine.

There are some things to think about though:

1. The code you use to invoke the delegates is not thread-safe. If one thread called WriteLogMessage() and another thread set logOutput to null at the same time, you might get a NullReferenceException. The thread-safe version would be:

   var logOutputTmp = this.logOutput;
   if (logOutputTmp != null)
       logOutputTmp(string.Format(format, args));
   

2. Consider using automatic properties. They do the same thing as your code, only with less writing. Also, get accessor is usually written before the set accessor, but that's only a minor style issue.

   public Action<string> LogOutput { get; set; }
   

3. Do you need the public setters and the parameterless constructor? If you expect that both delegates will be always set at construction, then it's better to make that clear and have only one constructor and no public setters.