Enhance MVC implementation

Question

I know that there are several ways to develop the MVC pattern. To improve consistently my knowledge, I would like to get some advice and feedback. How could I enhance my code? I knew that there are several lines of code, but it would be great if you help me improve my skills.

The Model store the object Person and notify the view if there are any changes. The view implements the interface IModelPersonObserver to receive the notification. The view notify the controller about any changes. The controller starts a new task for long calculations. The result is passed to the model.

To pass different events I am able to use multiple events, like ModelHandler and ViewHandler based on the real action or just one event and extend the EventArgs class.

IModelPerson

public delegate void ModelHandler<IModelPerson>(IModelPerson sender, ModelEventArgs e);

public class ModelEventArgs : EventArgs
{
    public Person person;
    public ModelEventArgs(Person p) { person = p; }
}

public interface IModelPersonObserver
{
    void Add(IModelPerson model, ModelEventArgs e);
}

public interface IModelPerson : IModel
{
    void Add(Person person);

    void Remove(Person person);

    void Observe(IModelPersonObserver obsever);
}

ModelPerson

public class ModelPerson : IModelPerson
{
    private readonly ArrayList persons = new ArrayList();

    public event ModelHandler<ModelPerson> change;

    public void Add(Person person)
    {
        this.persons.Add(person);
        this.change.Invoke(this, new ModelEventArgs(person));
    }

    public void Remove(Person person)
    {
        this.persons.Remove(person);
    }

    public void Observe(IModelPersonObserver obsever)
    {
        this.change += obsever.Add;
    }
}

IView

public interface IView
{
    Controller Controller { set; }    
}

IViewPerson

public delegate void ViewHandler<IViewPerson>(IViewPerson sender, ViewEventArgs e);

public class ViewEventArgs: EventArgs
{
    public Person person;
    public ViewEventArgs(Person p) { person = p; }
}

public interface IViewPerson : IView
{
    event ViewHandler<IViewPerson> change;
}

ViewPerson

public partial class ViewPerson : Form, IViewPerson, IModelPersonObserver
{
    private Controller controller;

    public event ViewHandler<IViewPerson> change;

    public ViewPerson()
    {
        InitializeComponent();
    }

    public Controller Controller
    {
        set { this.controller = value; }
    }

    private void button1_MouseClick(object sender, MouseEventArgs e)
    {
        var person = new Person();
        person.Name = "Name";
        person.Surname = "Surname";

        this.change.Invoke(this, new ViewEventArgs(person));
    }

    public void Add(IModelPerson model, ModelEventArgs e)
    {
        MessageBox.Show("e");
    }
}

Controller

public abstract class Controller
{
    protected readonly IModel model;

    protected readonly IView view;

    protected Controller(IModel model, IView view)
    {
        this.model = model;
        this.view = view;

        this.view.Controller = this;
    }
}

ControllerPerson

public class ControllerPerson : Controller
{
    private static ModelPerson modelPerson;

    public ControllerPerson(ModelPerson model, ViewPerson view) : base(model, view)
    {
        ControllerPerson.modelPerson = model;
        ControllerPerson.modelPerson.Observe(view);

        view.change += ViewOnChange; 
    }

    private static void ViewOnChange(IViewPerson sender, ViewEventArgs viewEventArgs)
    {
        Task.Factory.StartNew(() => Add(viewEventArgs.person));
    }

    private static void Add(Person state)
    {
        Thread.Sleep(5000);
        ControllerPerson.modelPerson.Add(state);
    }
}

What do you think?

Answer 1

public interface IView
{
    Controller Controller { set; }    
}

NO!!

You have just coupled the View with the Controller, which defeats the entire purpose.

This is also broken:

public abstract class Controller
{
    protected readonly IModel model;

    protected readonly IView view;

    protected Controller(IModel model, IView view)
    {

Now you have a circular dependency, with the controller knowing about the view, and the view knowing about the controller - but the controller knowing about the view and model is fine:

---

I don't understand this line:

Thread.Sleep(5000);

Why sleep for five whole seconds?

---

I'll just add that the naming convention, as noted by BCdotNET, is off: ControllerPerson should be PersonController - not because that's how ASP.NET MVC has it, but because C# and the .NET framework in general does it that way; it's just a matter of consistency... and it's what someone maintaining your code would expect to see.