MVP Pattern used in a payroll solution

Question

I'm using MVP pattern for a payroll system as my final year project. There are about 16 entity classes (like employee, client, salary etc.) in the analyzed problem and I have designed the solution as follows...

• Number of UIs (Views) – 18
• Separate Presenters and Models for each UI ( 18+18)
• One Data Service (repository) catering to all presenters

Each view, model and the data service implement interfaces and use dependency injection (constructor Inj).

Already I have implemented one UI and below is the code. This code compiles and runs giving the desired output.

My questions are:

1. What would be the consequences of applying this pattern, at the end of the project good? or bad? (As I don’t have a previous experience)

2. Any considerable flows in this pattern or in the code?

3. Is this pattern suitable for a project at this level?

4. Any advice on improving this?

Please note that my final ambition is to finish the project with the schedule and to pass the evaluation. So actually now what I want is to build confidence that I’m going the right direction.

View

namespace Payroll
{
 public delegate void ViewEventHandler(object sender, EventArgs e);
 public partial class frmNewClient : Form, IClientView
 {
 //Properties
 public int CID
 {
 get {return Convert.ToInt32(txtCID.Text); }
 set { txtCID.Text=value.ToString();}
 }
 public string ClientName
 {
 get { return txtCName.Text; }
 set { txtCName.Text = value; }
 }
.
.
.
 public frmNewClient()
 {
 InitializeComponent();
 }
 //Raise events, first to update model and then to save data to database
 private void cmdSave_Click(object sender, EventArgs e)
 {
 DataChanged(sender, e);
 Save(sender, e);
 }
 //Even handlers 
 public event EventHandler DataChanged;
 public event EventHandler Save;
 public event EventHandler Search;
 }
}

Model

namespace Payroll
{
 class Client: IClient
 {
 //Properties
 public int CID { get; set; }
 public string ClientName { get; set; }
 public string ClientAdd { get; set; }
 public string ContactPerson { get; set; }
 public string ClientMob { get; set; }
 public string ClientTel { get; set; }
 public string Remarks { get; set; }
 public DateTime EnteredDate { get; set; }
 //Custom constructor with parameteres
 public Client(int CID, string clientName, string clientAdd, string contactPerson, string clientMob, string clientTel, string Remarks, DateTime enteredDate)
 {
 this.CID = CID;
 this.ClientName = clientName;
 this.ClientAdd = clientAdd;
 this.ContactPerson = contactPerson;
 this.ClientMob = clientMob;
 this.ClientTel = clientTel;
 this.Remarks = Remarks;
 this.EnteredDate = enteredDate;
 }
 //Default constructor
 public Client()
 {
 }
 }
}

Presenter

namespace Payroll
{
 class ClientPresenter
 {
 IClient _model;
 IClientView _view;
 IDataService _ds;
 //Constructor 
 public ClientPresenter( IClient model,IClientView view, IDataService ds)
 {
 this._ds = ds;
 this._model = model;
 this._view = view;
 this.SetViewPropertiesFromModel();
 this.WireUpViewEvents();
 }
 //This will wire up the custom method provided to event
 private void WireUpViewEvents()
 {
 this._view.DataChanged +=new EventHandler(_view_DataChanged);
 this._view.Save += new EventHandler(_view_Save);
 this._view.Search += new EventHandler(_view_Search);
 }
//Following code snippet will sinc the parameters of the view with the parameters of the model and vice versa. (All text boxes in the UI are encapsulated in properties)
//====================================================================================== 
 private void SetModelPropertiesFromView()
 {
 foreach (PropertyInfo viewProperty in this._view.GetType().GetProperties())
 {
 if (viewProperty.CanRead)
 {
 PropertyInfo modelProperty = this._model.GetType().GetProperty(viewProperty.Name);
 if (modelProperty != null && modelProperty.PropertyType.Equals(viewProperty.PropertyType))
 {
 object valueToAssign = Convert.ChangeType(viewProperty.GetValue(this._view, null), modelProperty.PropertyType);
 if (valueToAssign != null)
 {
 modelProperty.SetValue(this._model, valueToAssign, null);
 }
 }
 }
 }
 }
 private void SetViewPropertiesFromModel()
 {
 foreach (PropertyInfo viewProperty in this._view.GetType().GetProperties())
 {
 if (viewProperty.CanWrite)
 {
 PropertyInfo modelProperty = this._model.GetType().GetProperty(viewProperty.Name);
 if (modelProperty != null && modelProperty.PropertyType.Equals(viewProperty.PropertyType))
 {
 object modelValue = modelProperty.GetValue(this._model, null);
 if (modelValue != null)
 {
 object valueToAssign = Convert.ChangeType(modelValue, viewProperty.PropertyType);
 if (valueToAssign != null)
 {
 viewProperty.SetValue(this._view, valueToAssign, null);
 }
 }
 }
 }
 }
 }
//====================================================================================== 
 private void _view_DataChanged(object sender, EventArgs e)
 {
 this.SetModelPropertiesFromView(); // Updating the model properties when data in the view properties changed...
 }
 private void _view_Save(object sender, EventArgs e)
 {
 this.Save();
 }
 private bool Save()
 {
 return _ds.InsertClient(_model);
 }
 private void _view_Search(object sender, EventArgs e)
 {
 this.Search();
 }
 private void Search()
 {
 var obj = _ds.GetByCID(_model.CID);
 if (obj != null)
 {
 _model = obj;
 this.SetViewPropertiesFromModel(); //Updating the view properties from the model if an object returned from GetByCID() method
 }
 }
 }
}

DataService

namespace Payroll
{
 class DataService :IDataService
 {
 public bool InsertClient(IClient newClient)
 {
 string InsertStatement = @"BEGIN INSERT INTO client(Client_Name, C_Add, Contact_Person, C_Mob_No, C_Tel_No, Remarks, Ent_Date)" +
 " VALUES (@CName, @CAdd, @CPerson, @CMob, @CTel, @Remarks, @entDate) END";
 using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
 {
 using (SqlCommand SqlCom = new SqlCommand(InsertStatement,newCon))
 {
 try
 {
 SqlCom.Parameters.Add("@CName", SqlDbType.VarChar).Value = newClient.ClientName;
 SqlCom.Parameters.Add("@CAdd", SqlDbType.VarChar).Value = newClient.ClientAdd;
 SqlCom.Parameters.Add("@CPerson", SqlDbType.VarChar).Value = newClient.ContactPerson;
 SqlCom.Parameters.Add("@CMob", SqlDbType.Char).Value = newClient.ClientMob;
 SqlCom.Parameters.Add("@CTel", SqlDbType.Char).Value = newClient.ClientTel;
 SqlCom.Parameters.Add("@Remarks", SqlDbType.VarChar).Value = newClient.Remarks;
 SqlCom.Parameters.Add("@entDate", SqlDbType.Date).Value = newClient.EnteredDate;
 SqlCom.Connection = newCon;
 newCon.Open();
 SqlCom.ExecuteNonQuery();
 return true;
 }
 catch (Exception e) { MessageBox.Show(("Error: " + e.Message)); }
 if (newCon.State == System.Data.ConnectionState.Open) newCon.Close();
 return false;
 }
 }
 }
 public Client GetByCID(int CID)
 {
 string SelectStatement=@"SELECT * FROM client WHERE CID=@ID";
 using (SqlConnection NewCon = new SqlConnection(db.GetConnectionString))
 { 
 using (SqlCommand SqlCom = new SqlCommand(SelectStatement,NewCon))
 {
 SqlCom.Parameters.Add("@ID", SqlDbType.Char).Value = CID;
 try
 {
 NewCon.Open();
 using (SqlDataReader NewRdr = SqlCom.ExecuteReader())
 {
 if (NewRdr.Read())
 {
 return new Client
 {
 CID = NewRdr.GetInt32(0),
 ClientName = NewRdr.GetString(1),
 ClientAdd = NewRdr.GetString(2),
 ContactPerson = NewRdr.GetString(3),
 ClientMob = NewRdr.GetString(4),
 ClientTel = NewRdr.GetString(5),
 Remarks = NewRdr.GetString(6),
 EnteredDate = NewRdr.GetDateTime(7)
 };
 }
 return null;
 }
 }
 catch (Exception e) { MessageBox.Show(("Error: " + e.Message)); }
 if (NewCon.State == System.Data.ConnectionState.Open) NewCon.Close();
 return null; 
 } 
 }
 }
 }
}

Programme

private void btnClient_Click(object sender, EventArgs e)
{
 IClient Client = new Client();
 IClientView ClientView = new frmNewClient();
 IDataService ds = new DataService();
 new ClientPresenter(Client, ClientView, ds);
 ClientView.ShowDialog();
}

Answer 1

View

//Even handlers 
public event EventHandler DataChanged;
public event EventHandler Save;
public event EventHandler Search;

Comments like this only clutter up the code, they add nothing that the code doesn't say already. Avoid comments that say "what", do use comments that say "why". Don't use comments to "sectionize" code, use vertical whitespace instead.

I've never implemented a MVP solution (switched to WPF/MVVM before I could!), but this definitely looks like it - your view is blissfully unaware of any business or data logic, and that's awesome; views should be exactly that. Your data service does nothing else, and the presenter coordinates everything... almost:

new ClientPresenter(Client, ClientView, ds);
ClientView.ShowDialog();

Since the view is a dependency for the presenter, I'd expect the presenter to pick up from its constructor and be responsible for displaying the view. It's also awkward that you're newing up a ClientPresenter but don't care about the object reference you've just created. I'd do something like this instead:

var presenter = new ClientPresenter(Client, ClientView, ds);
presenter.Show();

Where the presenter's Show method could be implemented by calling ShowDialog on the view, making the presenter... he who presents ;)

---

DataService

IDataService looks like some kind of repository - given that you're using ADO.NET, actually implementing a unit-of-work + repository pattern would have some benefits here, especially if you have more than just Client to deal with; you might be interested in a generic repository solution (there are caveats though):

public interface IRepository<T> where T : class
{
 T Select(int id);
}

Some entities may not be insertable or deletable, and some entities may not have an int key - that's why I'm not a fan of this pattern, but it can work in a lot of cases:

public interface IRepository<T> : IRepository<T> where T : class
{
 T Select(int id);
 bool Insert(T entity);
 // ...
}
public class ClientRepository : IRepository<Client>
{
 public Client Select(int id)
 {
 // ...
 }
 public bool Insert(Client entity)
 {
 // ...
 }
 // ...
}

A better solution would be to use Entity Framework's built-in unit-of-work and repositories, but I'm drifting. I like that you're using parameterized queries and properly disposing of disposables, but there's a nasty intrusion of a presentation concern in your data service:

catch (Exception e) 
{ 
 MessageBox.Show(("Error: " + e.Message)); 
}

I've put the catch clause on multiple lines for readability: code reads much better/faster going down than going across. The problem with having a MessageBox.Show call here, is that despite all the efforts you've put into dependency injection, you have introduced tight coupling with the MessageBox class and made the service class implement a presentation concern.

I'd just let whatever exception gets thrown in that class, bubble up to the presenter, who can then decide whether to display a potentially cryptic message to the user, or log the exception with its stack trace.

---

Presenter

I like that your private fields have an underscore prefix:

 IClient _model;
 IClientView _view;
 IDataService _ds;

However I'd make the intent much clearer like this:

 private IClient _model;
 private readonly IClientView _view;
 private readonly IDataService _ds;

This way whatever you do in the future, it's clear that the _view and _ds references cannot be tampered with, and that the _model reference can change during the object's lifetime, without even looking at the code.

The underscode prefix makes the this qualifier perfectly redundant, and IMHO clutter up the constructor's code:

 public ClientPresenter(IClient model,IClientView view, IDataService ds)
 {
 _ds = ds;
 _model = model;
 _view = view;
 SetViewPropertiesFromModel();
 WireUpViewEvents();
 }

Looking at SetViewPropertiesFromModel and SetModelPropertiesFromView implementations, I don't understand the need to use reflection cleverness. Such code would possibly be fine in a PresenterBase abstract class, but here you have a ClientPresenter that knows it's dealing with an IClient and an IClientView, both likely to expose properties that you can access directly. If you have 18 such presenters, it looks like you might have this code block copy+pasted 18 times... I'd look into refactoring some of it into a base abstract class.

• CAD: In this case is it better to use separate repositories for each model (like ClientRepository,EmployeeRepository... ) over general one (DataService)? Dividing things in this way(16 presenters,16 DataServices etc.) would be a mess in future development or will it ease this?
• Mathieu Guindon: Would you rather maintain 16 objects that do 1 thing, or 1 object that does 16 things? ;)
• CAD: One object that does one thing... ;) So if you have 16 things to do, so 16 objects. With regard to repository difficulty is to imagine whether it should be considered as doing one thing or 16 things as it is giving it's services to many clients?

Answer 2

Fragile SQL

string SelectStatement=@"SELECT * FROM client WHERE CID=@ID";
//....
ClientName = NewRdr.GetString(1),
ClientAdd = NewRdr.GetString(2),

SELECT * is very fragile. It is even more so when used with column access by index. SELECT * is for human use only, do not use it in your applications.

Naming

Do not prefix the name of a class to its members' names. Client.ClientName etc. It reduces readability.

Do not mangle names. Client.ClientAdd should be Client.Address.

using (SqlDataReader NewRdr = SqlCom.ExecuteReader()). Here NewRdr should start with lowercase because it's a local variable. It's not clear what is New about it. And Rdr is mangled. Since you are using it as a IDataReader mainly, you could name it dataReader.

Refactored Code

string selectStatement = @"SELECT NAME, ADDRESS, ... FROM client WHERE CID=@ID";
//....
Name = (string) dataReader["NAME"],
Address = (string) dataReader["ADDRESS"],