6
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ I'm not a fan of the naming style. Things like Controller, View etc. should be at the end of the name, not the start. "ViewPerson" sounds like a kind of Person, not a View. "ViewOnChange" is also odd. \$\endgroup\$ – BCdotWEB Oct 13 '14 at 15:03
  • \$\begingroup\$ I did that to order the files in the IDE, but you are right. Today I would change the name of those classes. \$\endgroup\$ – Andre Hofmeister Jan 16 '15 at 5:47
5
\$\begingroup\$
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:

controller knows about model and view, view knows about model, model knows nothing


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.

\$\endgroup\$
  • \$\begingroup\$ That was a long time ago. I think I added the Thread.Sleep to check if the UI does not block on longer tasks. You are correct with the circular dependency, I already fixed that. Thanks a alot. \$\endgroup\$ – Andre Hofmeister Jan 16 '15 at 5:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.