CODEWITHSUNDEEP
c#.netwinforms
Zenwalker
Zenwalker

Reputation: 1919

Mandatory Event not subscibed

Problem:

I am working on a application where in for some time consuming operation, i am supposed to show a progress bar on a form (WinForm) with a cancel button. So obviously i am using BackgroundWorker thread for it. Below is the code which simulates roughly of what i am trying to achieve.

namespace WindowsFormsApplication1
{
    public delegate void SomeDelegateHandler();

    public partial class Form1 : Form
    {
        public event SomeDelegateHandler DoSomeAction;
        BackgroundWorker bgWorker;

        public Form1()
        {
            InitializeComponent();

            bgWorker = new BackgroundWorker();
            bgWorker.DoWork += new DoWorkEventHandler(bgWorker_DoWork);
        }

        void bgWorker_DoWork(object sender, DoWorkEventArgs e)
        {
            //Some logic code here.
            for (int i = 0; i < 100; i++)
            {
                DoSomeAction();
            }
        }       

        private void Form1_Shown(object sender, EventArgs e)
        {
            if (DoSomeAction != null)
                bgWorker.RunWorkerAsync();
            else throw new EventNotSubscribedException();//Is this a valid style??
        }
    }

    public class EventNotSubscribedException : ApplicationException
    {
       //Some custom code here
    }
}

My Solution

As per the above code, as soon as the form is displayed to the user (OnShown event) i am starting the backgroundworker thread. This is because, the user need not to initiate any action for this to happen. So onshown does time consuming operation job. But the issue is, as i have shown above, the main time consuming job is executed on other class/component where it is kind of tight bounded too (legacy code: cant refactor). Hence i have subscribed to the event DoSomeAction in that legacy code class which launches this form.

Doubt/Question:

Is it valid to throw exception as shown above? (Please read my justification below).

Justification:

The OnShown event does check for null on event handler object. This is because, to make this form usable, the event has to be subscribed by the subscriber (usage code), then only it shall work. If not, then the form just displays and does noting at all and usage code may not know why it is happenings so. The usage code may assume that subscribing to the event is option just like button click events per say.

Hope my post is clear and understandable.

Thanks & Happy Coding, Zen :)

Upvotes: 1

Views: 113

Answers (2)

cdhowie
cdhowie

Reputation: 168998

I would suggest making the consuming code construct the BackgroundWorker and pass it to the form's constructor. You can do a null test in the constructor and side-step this whole issue. Alternatively, take the delegate as a constructor argument instead. I mean, how likely is it that the consuming code will need to change the worker delegate mid-operation?

Another approach is to have the dialog monitor a task, instead of having a dialog control a task (as you have here). For example, you could have an interface like this:

public interface IMonitorableTask {
    void Start();

    event EventHandler<TData> TaskProgress;
}

Where TData is a type that provides any information you might need to update the dialog (such as percent completed).

The downside to this is that each task needs to be a type of its own. This can lead to very ugly, cluttered code. You could mitigate that issue somewhat by creating a helper class, something like:

public class DelegateTask : IMonitorableTask {
    private Action<Action<TData>> taskDelegate;

    public event EventHandler<TData> TaskProgress;

    public DelegateTask(Action<Action<TData>> taskDelegate) {
        if (taskDelegate == null)
            throw new ArgumentNullException("taskDelegate");

        this.taskDelegate = taskDelegate;
    }

    protected void FireTaskProgress(TData data) {
        var handler = TaskProgress;

        if (handler != null)
            handler(this, data);
    }

    public void Start() {
        taskDelegate(FireTaskProgress);
    }
}

Then your task methods become factories:

public IMonitorableTask CreateFooTask(object argument) {
    return new DelegateTask(progress => {
        DoStuffWith(argument);

        progress(new TData(0.5));

        DoMoreStuffWith(argument);

        progress(new TData(1));
    });
}

And now you can easily(*) support, say, a command-line interface. Just attach a different monitor object to the task's event.

(*) Depending on how clean your UI/logic separation already is, of course.

Upvotes: 1

abhishek
abhishek

Reputation: 2992

Do you mean that you need to throw an exception to the caller of the form? Is it called using showDialog or Show?

BTW, I dont prefer to generate an exception from an event. Rather it would be rather nice to keep it such that it returns from the place with some status set on the Form class.

for instance, I would prefer using

IsEventSubscribed = false this.Close()

rather than EventNotSubscribedException

BTW, One problem I can see in the code, when the bgWorker_DoWork is called, you should check DoSomeAction to null, because otherwise it might cause NullReferenceException.

Preferably,

  1. Start the run the RunWorkerAsync from Form_shown
  2. Check Delegate to null in DoWork, if it is null, do not call DoSomeAction otherwise call it.
  3. On RunWorkerCompleted of the BackgroundWorker, close the form.

Let me know if you need anything more.

Upvotes: 1

Related Questions