Calculating Earnings from attendance details

Question

In a C# win forms payroll application I need to calculate employee salary based on their attendance. I've written a stored procedure which calculates earnings of all employees from the data available in attendance table. So when the application calls the SP the data will be shown on a data grid view and then immediately these data will be saved in SalaryTrans table.

Please note in following demo code I've excluded the use of Interfaces and DI.

Since this is an object oriented development employing MVP design pattern, could you please advice me whether the way I've done this acceptable?

I know that I've not used a Model, in this case SalaryModel as the DataService return a DataSet object with all earnings figures of all employees. So Could you please show me a way of using a Model (in this case SalaryModel) in this scenario if possible?

Form

public partial class frmSalary : Form
{
    public frmSalary()
    {
        InitializeComponent();
    }

    private void btnProcessEarning_Click(object sender, EventArgs e)
    {
        SalaryPresenter presenter= new SalaryPresenter();
        presenter.ShowEarnings();
        presenter.SaveEarnings();
    }
}

Presenter

class SalaryPresenter
{
    private void ShowEarnings()
    {
        _View.dgEmployeeSalary.DataSource = CalculateEarnings(dtpStartDate.Value,dtpEndDate.Value);
    }

    private void SaveEarnings()
    {
        _DataService.InsertEarnings(dgEmployeeSalary);
    }

    private DataTable CalculateEarnings(DateTime startDate, DateTime endDate)
    {
        return _DataService.Calculate(startDate, endDate);
    }
}    

Data Service

class DataService
{
    public DataTable Calculate( DateTime fromDate, DateTime toDate)
    {
        using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
        {
            using (SqlCommand sqlCommand = new SqlCommand("dbo.sp_Earnings", sqlConnection))
            {
                sqlCommand.CommandType = CommandType.StoredProcedure;
                sqlCommand.Parameters.Add("@fromDate", SqlDbType.DateTime).Value = fromDate;
                sqlCommand.Parameters.Add("@toDate", SqlDbType.DateTime).Value = toDate;
                using (SqlDataAdapter sqlAdapter = new SqlDataAdapter(sqlCommand))
                {
                    using (DataSet dataSet = new DataSet())
                    using (DataTable dataTable = new DataTable())
                    {        
                        sqlConnection.Open();
                        dataSet.Tables.Add(dataTable);
                        sqlAdapter.Fill(dataTable);
                        return dataTable;
                    }
                }
            }
        }
    }

    public void InsertEarnings(DataGrid dataGrid)
    {
        using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
        {
            foreach (DataGridViewRow row in dataGrid.Rows)
            {
                if (row.Cells[1].Value != null)
                {
                    string commandString = "IF NOT EXISTS (SELECT * FROM SalaryTrans WHERE emp_id=@emp_id) BEGIN INSERT INTO salaryTrans..."
                    //Database transaction code here
                }
            }
        }
    }
}

Answer 1

General point
Is there a strong reason for going with MVP? One of the downsides of an MVP approach is that a lot of the nice data-binding facilities within the architecture get bypassed. A Presentation Model / Model-View-ViewModel (MVVM) approach is pushed for WPF and can be used with Winforms.

There is a nice write-up on MVP to be found at MVC vs. MVP.

Form / Presenter
Whichever flavour of MVx one goes with, a basic idea is that the View doesn't need to know about the Presenter (and vice versa). You mentioned excluding the interfaces and DI but in the Form code, it still needs to know about the Presenter class - its method signatures et al - and the Presenter knows about each of the controls on the Form.

Another way of structuring this is to create an interface

interface ISalaryFormView {
    event EventHandler<EventArgs> RequestProcessEarnings;
    void SetSalaryDetails(IEnumerable<SalaryDetails> salaryDetailsList);
    DateTime StartDateTime { get; set; }
    DateTime EndDateTime { get; set; }
}

(The SetSalaryDetail() method and StartDateTime, EndDateTime properties are part of the reason why MVVM is being pushed. A lot less plumbing code)

and have the Presenter take an ISalaryFormView as an argument

class SalariesPresenter {

    private readonly ISalaryForm _form;
    private readonly SalariesService _service;

    public SalariesPresenter(ISalaryForm form, SalariesService service) {
        _form = form;
        _service = service;
        _form.ProcessEarningsRequested += HandleProcessEarningsRequested;
    }

    private void HandleProcessEarningsRequested(object sender, EventArgs e) {
        _form.SetSalaryDetails(_service.GetSalaries(_form.StartDateTime, _form.EndDateTime));
    }
}

In the form, we now implement the interface and rout the click to raising the event

public partial class MainWnd : Form, ISalaryForm  {

    public MainWnd() {
       InitializeComponent();
    }

    private void btnProcess_Click(object sender, EventArgs e) {
        RaiseProcessEarningsRequested();
    }

    #region ISalaryForm

    public void SetSalaryDetails(IEnumerable<SalaryDetails> salaryDetailsList) {
        dgvSalaries.DataSource = salaryDetailsList;
        dgvSalaries.Refresh();
    }

    public DateTime StartDateTime { 
        get { return dtpStartDate.Value; }
        set { dtpStartDate.Value = value; }
    }

    public DateTime EndDateTime {
        get { return dtpEndDate.Value; }
        set { dtpEndDate.Value = value; }
    }

    #region ProcessEarningsRequested

    public event EventHandler<EventArgs> ProcessEarningsRequested;

    protected void RaiseProcessEarningsRequested() {
        OnRaiseProcessEarningsRequested(EventArgs.Empty);
    }

    protected virtual void OnRaiseProcessEarningsRequested(EventArgs e) {
        var handler = ProcessEarningsRequested;
        if (handler != null) {
            handler(this, e);
        }
    }

    #endregion

    #endregion

}

We do this to help the testing in the Presenter

• It doesn't need to know the concrete class of the Form - it is working against an interface.
• It doesn't need to know the final screen format - swap out a listview for the datagrid and it doesn't impact.
• We can unit test by having a Mock raise and event and verifying that the Setter is called.

DataService
It is a personal preference but I have a dislike of passing around data in DataTables, especially untyped datatables. A quick web search will give lots of reasons against doing so. Adding data containers and reading into them doesn't add that much work. That said, it is a personal preference. The datatable is a lot faster and doesn't impact the MVx shape at all.

Passing the DataGrid in as a parameter though is a bad idea. We have now a linkage to System.Windows.Controls that is totally unnecessary and impacts portability (say we want to use this on the server side of a Web app or migrate to WPF). Even changing the screen layout - changing to a DataGridView instead of a DataGrid is impacted. Passing in a IEnumerable<> of some Data Transfer Object (DTO) (or even a datatable) is a better idea. No layer behind the view should know about or be concerned with the screen controls.

EDIT
Generally I would return an IEnumerable<T> rather than an IList<T> but in the example I am working on this is giving strange results => the DataGridView is not refreshing correctly for IEnumerable<T> but works fine for IList<T>.

For the minimal example I have two columns, Name and Value,

I have a SalaryDetails class

public class SalaryDetails {
    public string Name { get; set; }
    public decimal Value { get; set; }
}

and the SalariesService class

public class SalariesService {

    public IList<SalaryDetails> GetSalaries(DateTime start, DateTime end) {
        return ReadTable(start, end).Rows.OfType<DataRow>().Select(CreateSalaryDetails).ToList();
    }

    private static DataTable ReadTable(DateTime start, DateTime end) {
        var ret = new DataTable();
        ret.Columns.Add("Name", typeof (string));
        ret.Columns.Add("Value", typeof(decimal));

        ret.Rows.Add(new object[] { "January", 1.1m});
        ret.Rows.Add(new object[] { "February", 1.21m });
        ret.Rows.Add(new object[] { "March", 1.331m });
        ret.Rows.Add(new object[] { "April", 1.4641m });
        ret.Rows.Add(new object[] { "May", 1.651051m });

        return ret;
    }

    private static SalaryDetails CreateSalaryDetails(DataRow row) {
        return new SalaryDetails() {Name = (string) row["Name"], Value = (decimal) row["Value"]};
    }

}

The ReadTable() is a placeholder for your current Calculate() call and the Select() call is a quick way to create the SalaryDetails entries from the datarows.

Notes
I hadn't looked over your Calculate code in detail (I felt that wasn't really part of the MVP question) but looking at it now I have a few comments.
1) AFAIK, you shouldn't need a DataSet. You should be able to create and fill the table without one - not worth it to me to set up a DB to try it out but you might.
2) The using block on the datatable looks a bit strange. On leaving the using block (i.e. when returning), Dispose() is called on the DataTable. This doesn't appear to do anything (the data is still in the DataTable) so it is probably worth leaving out. (A quick wander down through the DataTable code shows no specific implementation for Dispose, it uses the base MarshalByValueComponent version)

EDIT
There is an IsNull() method on the DataRow. One can use an extension to check for nulls and return default values

public static class DataRowExtensions {
    public static T GetValue<T>(this DataRow row, string name) {
        return row.IsNull(name)
                ? default(T)
                : row.Field<T>(name);
    }
}