5
\$\begingroup\$

I want to refactor this code that is currently in a partial Form Class and deals with edits made on certain cells in a datagridview (in this case called dgvReplenish).

I would like to refactor so that I can create unit tests to be able to test the logic that effects changing the back color of cells, and updating other cells.

The problem I am having is how to refactor logic to an external class, that has references to the winform controls.

Form

enter image description here

All following code is part of public partial class ReplenForm : Form

DataGridView Event Handlers

   private string lastEditedCellValue;

    private void dgvReplenish_CellBeginEdit(object sender, DataGridViewCellCancelEventArgs e)
    {
        lastEditedCellValue = dgvReplenish.Rows[e.RowIndex].Cells[e.ColumnIndex].Value.ToString();
    }

    private void dgvReplenish_CellValidating(Object sender, DataGridViewCellValidatingEventArgs e)
    {
        if (e.ColumnIndex == (int)ProductColumnIndex.ReplenishAmount)
        {
            ValidateEditOfReplenishAmount(e);
        }
    }

    private void dgvReplenish_CellClicked(object sender, DataGridViewCellEventArgs e)
    {
        PopulateSelectedLineDetails(e.RowIndex);
    }

    private void dgvReplenish_CellFormatting(object sender, DataGridViewCellFormattingEventArgs e)
    {
        ChangeColorOfWarehouseShopName(e);
    }

    private void dgvReplenish_CellValueChanged(object sender, DataGridViewCellEventArgs e)
    {
        if (e.ColumnIndex == (int)ProductColumnIndex.ReplenishAmount){

            OnReplenishAmountEdit(e.RowIndex, e.ColumnIndex);
            AddEditedRowToEditedRowsDictionary(e.RowIndex, e.ColumnIndex);
        }

        ChangeAvailableStockOnEditOfReplenishAmount(e.RowIndex, e.ColumnIndex);
    }

Cell Edit functions

 /// <summary>
    /// When Replenish Amount is edited change back colour of cell and update database
    /// </summary>
    /// <param name="e">DataGridView Cell Arguments object</param>
    private void OnReplenishAmountEdit(int rowIndex, int columnIndex)
    {
        var editedReplenishAmount = GetColumnValueForRow(
                rowIndex, 
                ProductColumnIndex.ReplenishAmount.ToString()
            );

        var originalReplenishAmount = GetColumnValueForRow(
                rowIndex, 
                ProductColumnIndex.OriginalReplenishAmount.ToString()
            );

        if (editedReplenishAmount != originalReplenishAmount)
        {
            GridViewHelper.ChangeBackColorOfCellOnEdit(rowIndex, columnIndex, dgvReplenish);
         //   UpdateDatabase();
        }
        else
        {
            GridViewHelper.RevertBackColorOfCellOnEdit(rowIndex, columnIndex, dgvReplenish);
        }
    }

    /// <summary>
    /// Checks if the amount entered for Replenish Amount is a number, 
    /// is less/equal to the free stock for that part. If so clears error messages, 
    /// if not adds error message to status bar
    /// </summary>
    /// <param name="e">DataGridView Cell Arguments object</param>
    private void ValidateEditOfReplenishAmount(DataGridViewCellValidatingEventArgs e)
    {
        if (!string.IsNullOrEmpty(lastEditedCellValue))
        {
            Int32 result;
            bool valueIsAnInteger = Int32.TryParse(e.FormattedValue.ToString(), out result);

            if (valueIsAnInteger)
            {
                ValidateCellEdit(e.RowIndex, int.Parse(e.FormattedValue.ToString()));
            }
            else
            {
                //Value Entered Is A Non Number
                ResetCellAndAddErrorMessage("A non number has been entered as the Replenish Amount");
            }
        }
    }

    /// <summary>
    /// Check if new cell value added to existing replenish amount total
    /// for that row's part is less or equal to that parts available stock.
    /// if not undo edit and return error
    /// </summary>
    /// <param name="rowIndex">Row Number</param>
    /// <param name="newValue">Edited Cells new value</param>
    private void ValidateCellEdit(int rowIndex, int newValue)
    {
        bool replenishTotalIsEqualToOrLessThanAvailableStock =
            IsReplenishTotalEqualOrLessToAvailableStock(rowIndex);

        if (replenishTotalIsEqualToOrLessThanAvailableStock)
        {
            lbStatusBar.ForeColor = Color.Black;
            lbStatusBar.Text = string.Empty;
        }
        else
        {
            //Value Entered Is More Than Free Stock
            ResetCellAndAddErrorMessage("Replenish Amount entered is more than the available free stock");
        }
    }

    /// <summary>
    /// Works out if the running total of Replenish amounts
    /// is less or equal to the available stock for that part.
    /// </summary>
    /// <param name="rowIndex">Row Number</param>
    /// <param name="newValue">Replenish amount value entered</param>
    /// <returns>True or false</returns>
    private bool IsReplenishTotalEqualOrLessToAvailableStock(int editedRowIndex)
    {
        var rows = GetRowsIndexAndValues(ProductColumnIndex.ReplenishAmount.ToString());

        int replenishAmountTotal = ReplenishEditHelper.CalculateReplenishAmountTotal(editedRowIndex, rows);

        var currentAvailableStock = GetColumnValueForRow(
               editedRowIndex, 
                ProductColumnIndex.OriginalAvailableStock.ToString()
            );

        if (replenishAmountTotal <= currentAvailableStock)
        {
            return true;
        }
        else
        {
            return false;
        }
    }



    /// <summary>
    /// Changes colour of cell if cell is in the ShopName column and 
    /// row has a StoreGrade type MailOrder to WareHouse
    /// </summary>
    /// <param name="e">DataGridView Cell Arguments object</param>
    private void ChangeColorOfWarehouseShopName(DataGridViewCellFormattingEventArgs e)
    {
        if (e.ColumnIndex == (int)ProductColumnIndex.ShopName)
        {
            var currentRowStoreGradeValue = GridViewHelper
                .GetCellValueAsInteger(
                    dgvReplenish,
                    e.RowIndex,
                    ProductColumnIndex.StoreGradeEnum.ToString()
                );

            var storeIsWarehouse = currentRowStoreGradeValue == (int)StoreGradeType.Warehouse;
            var storeIsMailOrder = currentRowStoreGradeValue == (int)StoreGradeType.MailOrder;

            if (storeIsWarehouse || storeIsMailOrder)
            {
                e.CellStyle.BackColor = Color.LightBlue;
            }
        }
    }

    /// <summary>
    /// Updates the Available Stock column for all rows that have the same Part
    /// </summary>
    /// <param name="editedRowIndex">Row Number</param>
    /// <param name="columnIndex">Column Number</param>
    private void ChangeAvailableStockOnEditOfReplenishAmount(int editedRowIndex, int columnIndex)
    {
        if (columnIndex == (int)ProductColumnIndex.ReplenishAmount)
        {
            var amountChangedBy = GetAmountTotalReplenishmentChangedBy(editedRowIndex);

            foreach (DataGridViewRow row in dgvReplenish.Rows)
            {
                var currentRowPartValue = GetRowPartValue(row.Index);

                var editedRowPartValue = GetRowPartValue(editedRowIndex);

                if (currentRowPartValue == editedRowPartValue)
                {
                    var currentAvailableStock = GetColumnValueForRow(
                            editedRowIndex, 
                            ProductColumnIndex.OriginalAvailableStock.ToString()
                        );

                    row.Cells[(int)ProductColumnIndex.AvailableStock]
                        .Value = currentAvailableStock - amountChangedBy;
                }
            }
        }
    }

    private int GetAmountTotalReplenishmentChangedBy(int rowIndex)
    {
        var rows = GetRowsIndexAndValues(ProductColumnIndex.ReplenishAmount.ToString());

        var newTotalReplenishAmount = ReplenishEditHelper.CalculateReplenishAmountTotal(rowIndex, rows);

        rows = GetRowsIndexAndValues(ProductColumnIndex.OriginalReplenishAmount.ToString());

        var currentTotalReplenishAmount = ReplenishEditHelper.CalculateReplenishAmountTotal(rowIndex, rows);

        var amountChangedBy = newTotalReplenishAmount - currentTotalReplenishAmount;

        return amountChangedBy;
    }

    private Dictionary<int,Tuple<string, int>> GetRowsIndexAndValues(string columnName)
    {
        int numOfRows = dgvReplenish.Rows.Count;
        Dictionary<int, Tuple<string, int>> rows = new Dictionary<int, Tuple<string, int>>();

        for (int i = 0; i < numOfRows; i++)
        {
            int rowIndex = dgvReplenish.Rows[i].Index;

            Tuple<string,int> rowValues = new Tuple<string,int>(
                GetRowPartValue(rowIndex),
                GridViewHelper
                    .GetCellEditedFormattedValueAsInteger(
                        dgvReplenish,
                        rowIndex,
                        columnName
                    )
            );
            rows[dgvReplenish.Rows[i].Index] = rowValues;
        }
        return rows;
    }

    /// <summary>
    /// Sets all visible Replenish Amount Cells to the value of tbUpdateAll
    /// </summary>
    private void UpdateAllReplenishAmountCells()
    {
        var updateValue = tbUpdateAll.Text;

        if (!string.IsNullOrEmpty(updateValue))
        {
            var columnIndex = (int)ProductColumnIndex.ReplenishAmount;

            bool canUpdateAllRows = CanAllRowsBeUpdated(updateValue, columnIndex);

            if (canUpdateAllRows)
            {
                foreach (DataGridViewRow row in dgvReplenish.Rows)
                {
                    row.Cells[columnIndex].Value = updateValue;
                }
            }
        }
    }

    /// <summary>
    /// Loops through rows visible in grid and checks to see if there is available
    /// stock for each part to change each row's replenish amount to the passed value
    /// </summary>
    /// <param name="updateValue">Value for each cell to be updated to</param>
    /// <param name="columnIndex">Column Number</param>
    /// <returns>True or false</returns>
    private bool CanAllRowsBeUpdated(string updateValue, int columnIndex)
    {
        foreach (DataGridViewRow row in dgvReplenish.Rows)
        {
            bool replenishTotalIsEqualToOrLessThanAvailableStock =
             IsReplenishTotalEqualOrLessToAvailableStock(row.Index);

            if (replenishTotalIsEqualToOrLessThanAvailableStock)
            {
                row.Cells[columnIndex].Value = updateValue;
            }
            else
            {
                StatusBarHelper.SetErrorMessage(
                        lbStatusBar,
                        "Cannot update all rows with amount as not enough available stock"
                    );

                return false;
            }
        }
        return true;
    }

Helpers

/// <summary>
        /// Resets Cell Value and adds an error message to status bar
        /// </summary>
        private void ResetCellAndAddErrorMessage(string message)
        {
            ResetEditedCell();

            StatusBarHelper.SetErrorMessage(
                    lbStatusBar, 
                    message
                );
        }

        /// <summary>
        /// Reset edited cell to the original value before edit
        /// </summary>
        private void ResetEditedCell()
        {
            if(dgvReplenish.IsCurrentCellInEditMode){
                dgvReplenish.EditingControl.Text = lastEditedCellValue;
            }
        }

        /// <summary>
        /// Get the Part cell value for a particular row
        /// </summary>
        /// <param name="rowIndex">Row Number</param>
        /// <returns>Part cell value</returns>
        private string GetRowPartValue(int rowIndex)
        {
            return GridViewHelper
                   .GetCellValueAsString(
                       dgvReplenish,
                       rowIndex,
                       ProductColumnIndex.Part.ToString()
                   );
        }

        /// <summary>
        /// Gets the column value as an integer for a given row and column
        /// </summary>
        /// <param name="rowIndex">Row Number</param>
        /// <param name="columnName">Column Number</param>
        /// <returns>Column Value</returns>
        private int GetColumnValueForRow(int rowIndex, string columnName)
        {
          return GridViewHelper
                .GetCellValueAsInteger(
                    dgvReplenish,
                    rowIndex,
                    columnName
                );
        }

        /// <summary>
        /// Gets the row ID for the row
        /// </summary>
        /// <param name="rowIndex">Row Number</param>
        /// <returns>Row ID</returns>
        private int GetRowID(int rowIndex)
        {
            var row = this.dgvReplenish.Rows[rowIndex];

            return (row.Cells[(int)ProductColumnIndex.ID].Value == null) ? 0 :
                   (int)row.Cells[(int)ProductColumnIndex.ID].Value;
        }

Databinding

private DataTable dataTable;
        private ProductDataSet dataSet;

        /// <summary>
        /// Add datasource and set Column Display settings
        /// </summary>
        private void InitialiseDataGridView()
        {
            RefreshData();

            if (dataSet.HasDataSetCreationFailed() == false)
            {
                dgvReplenish.AutoGenerateColumns = false;

                //Make sure all cells autosize for width
                dgvReplenish.AutoSizeColumnsMode = DataGridViewAutoSizeColumnsMode.AllCells;

                // numeric columns need to be right aligned, and boolean ones centred
                GridViewHelper.SetColumnContentAlignment(dgvReplenish);

                // Set Display and edit settings for each column
                SetColumnDisplaySettings();

                dgvReplenish.Update();

                isDataGridIntialised = true;

                Add_dgReplenish_EventHandlers();
            }
            else
            {
                DataSetCreationHasFailed();
            }
        }

        /// <summary>
        /// Refreshes data for Gridview from source dataset
        /// </summary>
        private void RefreshData()
        {
            if (dataSet != null)
            {
                dataSet = null;
            }

            CreateDataSet();

            if (dataSet.HasDataSetCreationFailed() == false)
            {
                SetGridDataSource();
            }
            else
            {
                DataSetCreationHasFailed();
            }
        }

        /// <summary>
        /// Assisgns refreshed DataTable to the DataGridView's datasource
        /// </summary>
        private void SetGridDataSource()
        {
            this.dataTable = dataSet.GetDataTable();

            dgvReplenish.DataSource = this.dataTable;
        }

        /// <summary>
        /// Sets a boolean that the dataset has failed to initialise 
        /// and adds error to status bar
        /// </summary>
        private void DataSetCreationHasFailed()
        {
            isDataGridIntialised = false;
            StatusBarHelper.SetErrorMessage(lbStatusBar, dataSet.ErrorInDataSetCreation);
        }

        /// <summary>
        /// Refreshes the dataset from its source data
        /// </summary>
        private void CreateDataSet()
        {
          //  dataSet = new ProductDataSet(new DBRplenEditImport());
            dataSet = new ProductDataSet(new CSVDataImport());
            dataSet.SetDataList();
        }

        /// <summary>
        /// For each column set read only, if to display on grid, 
        /// sortmode to not sortable and column header to use the 
        /// datacolumn caption rather than name
        /// </summary>
        private void SetColumnDisplaySettings()
        {
            for (var i = 0; i < dgvReplenish.Columns.Count; i++)
            {
                GridViewHelper.SetColumnToReadOnly(i, dgvReplenish);

                GridViewHelper.SetColumnNoDisplayOnGrid(i, dgvReplenish);

                dgvReplenish.Columns[i].SortMode = DataGridViewColumnSortMode.NotSortable;

                // Use caption for header text rather than column name
                // as Name uses the product property name which has no formatting
                dgvReplenish.Columns[i].HeaderText = dataTable.Columns[dgvReplenish.Columns[i].HeaderText].Caption;
            }
        }

        /// <summary>
        /// Set up all the event handlers that will be used in the DataGridView
        /// </summary>
        private void Add_dgReplenish_EventHandlers()
        {
            dgvReplenish.CellBeginEdit -= new DataGridViewCellCancelEventHandler(dgvReplenish_CellBeginEdit);
            dgvReplenish.CellBeginEdit += new DataGridViewCellCancelEventHandler(dgvReplenish_CellBeginEdit);
            dgvReplenish.CellValidating -= new DataGridViewCellValidatingEventHandler(dgvReplenish_CellValidating);
            dgvReplenish.CellValidating += new DataGridViewCellValidatingEventHandler(dgvReplenish_CellValidating);
            dgvReplenish.CellValueChanged -= new DataGridViewCellEventHandler(dgvReplenish_CellValueChanged);
            dgvReplenish.CellValueChanged += new DataGridViewCellEventHandler(dgvReplenish_CellValueChanged);
            dgvReplenish.CellFormatting -= new DataGridViewCellFormattingEventHandler(dgvReplenish_CellFormatting);
            dgvReplenish.CellFormatting += new DataGridViewCellFormattingEventHandler(dgvReplenish_CellFormatting);
            dgvReplenish.CellClick -= new DataGridViewCellEventHandler(dgvReplenish_CellClicked);
            dgvReplenish.CellClick += new DataGridViewCellEventHandler(dgvReplenish_CellClicked);
            dgvReplenish.DataBindingComplete -= new DataGridViewBindingCompleteEventHandler(dgvReplenish_DataBindingComplete);
            dgvReplenish.DataBindingComplete += new DataGridViewBindingCompleteEventHandler(dgvReplenish_DataBindingComplete);
        }
\$\endgroup\$
  • \$\begingroup\$ Is all of this code inside the same partial class? \$\endgroup\$ – RubberDuck Jun 5 '15 at 13:52
  • \$\begingroup\$ Also, is there any chance of getting a screenshot of the form? \$\endgroup\$ – RubberDuck Jun 5 '15 at 13:53
  • \$\begingroup\$ yes its all currently in the same class \$\endgroup\$ – Peter Campbell Jun 5 '15 at 13:53
  • 1
    \$\begingroup\$ I've added image, also note I have not posted the entire partial class, as quite large, only the section I am interested in refactoring \$\endgroup\$ – Peter Campbell Jun 5 '15 at 13:58
  • \$\begingroup\$ One last question. How is the data binding occurring? Is that code here somewhere, or did you do that through the designer? \$\endgroup\$ – RubberDuck Jun 5 '15 at 14:07
5
\$\begingroup\$

The truth is, it's really very hard to refactor this code out of the code behind once it's been written like this. I would suggest that you start writing your tests first, for now at least. The benefit there is that you have to design your code to be testable to begin with. If you decide later (once you have an idea of how to better design your code), to test after the fact, that's fine. The important thing right now is to separate the concerns.

You need some Model-View-Presenter in your life. It's more or less the default design pattern for WinForms. There are two flavors, passive and supervising controller.

MVP Diagrams

The very first thing you need to do is make sure that your form is not responsible for retrieving the DataTable. You will never be able to test this code if the form is responsible for retrieving the data. It needs to be provided the data via a constructor or property. The Presenter should be responsible for providing it to the View. Most likely, it will delegate this responsibility to another class, but ultimately, the Presenter will pass the information from that class to the view.

You'll also need to properly model the data. Right now, there's no concept of a Part in this code, but there's obviously a business object that is very much a Part and your form displays a list of Parts. I'd start with defining a simple class defining the data structure itself, and add logic to it as necessary. Eventually, this is where your validation logic will lie.

internal class Part
{
    int Id { get; private set; } //consider wrapping this in a simple PartId wrapper around int.
    string Description { get; private set; }
    byte Rsp { get; private set; }
    string Identifier { get; private set; }

    // lots more properties

    internal Part(/* one param for each property */)
    {
      //...
    }
}

And now, to define an interface for your view. (Note that I left everything but what you need for the data grid out.)

internal interface IPartsView
{
    IList<Part> Parts { get; set; }
}

So, your Form's signature will need to change a bit to implement this.

public partial class ReplenForm : Form, IPartsView

Then in the Getter/Setter, you'll transform the list into something bindable (personally, I like the BindingList) and bind it to the datagrid. There's a great example of binding a list to a datatable and datagridview on dotnetpearls.

So, we've gotten all of the from outside of the form and injected it in. Great, but what about all those pesky events? Well, we'll add some to the interface for the presenter to respond to. The Form will just delegate it's own events by raising IPartsView's events.

For example, let's handle a selected record change.

internal interface IPartsView
{
    IList<Part> Parts { get; set; }

    event EventHandler SelectedItemChanged;
}

Back in your form you raise the interface's event whenever the DataGridView's event is raised.

public EventHandler SelectedItemChanged;
private void dgvReplenish_CellClicked(object sender, DataGridViewCellEventArgs e)
{
    var handler = SelectedItemChanged;
    if (handler != null)
    {
        SelectedItemChanged(this, e);
    } 
}

Which moves PopulateSelectedLineDetails into your presenter. It catches the raised event, and interacts with the View's interface, performing this work.

This means, that you can now relatively simply test your code. I like using Moq for this. Moq makes it easy to mock out the view and test the presenter. Because that's what we're really aiming for here. Your Form (IPartsView) needs to be dead dumb simple. It shouldn't need to be tested. If the presenter and model do all of the work, then there's no need to test the view.

A simple test could look something like this.

[TestMethod]
public void FieldXIsUpdatedAfterSelectionChange()
{
    //arrange

    var parts = //some code to create a dummy parts list

    var view = new Mock<IPartsView>();
    view.SetupAllProperties();

    var presenter = new PartsPresenter(view.Object, parts);

    //act 

    //simulate selection change
    view.Raise(v => v.SelectedItemChanged += null, new CustomEventArgs(2)); // row 2

    //assert
    Assert.AreEqual(expected, view.SomeProperty);
}

I know that was a lot to take in, and not a whole lot of actual code from your project, but you really do have yourself in a pickle here. I recommend implementing something simple with this pattern before trying to refactor to it with this monolithic code behind that you have there. I fear that if you try without having a simpler experience first, that you'll fail and never learn how powerfully awesome this pattern can be in your code.

Good luck!

\$\endgroup\$
  • 1
    \$\begingroup\$ I totally wanted to write this first \$\endgroup\$ – Robert Snyder Jun 5 '15 at 15:12
  • 1
    \$\begingroup\$ @RobertSnyder feel free to expand in another answer. I feel like there's a lot more to be covered, but I had to cut it off at some point and get on with my day. \$\endgroup\$ – RubberDuck Jun 5 '15 at 15:13
4
\$\begingroup\$

To expand on RubberDucks answer, about adding tests first is great! In fact you should create a class that only does CRUD operations on a Part. (if you didn't know CRUD is short for Create, Read, Update, Delete. It is the typical operations performed on set of data) You can then test the implementation of your PartRepository before you even put it to use in production. Here is a basic interface that you could implement that does all the necessary parts. With the repository defined you would then implement it with PartRepository and use that in production.

public interface IPartRepository
{
    IEnumerable<Part> GetAllParts();
    Part GetPart(int id);
    void DeletePart(int id);
    void UpdatePart(int id, Part partToUpdateWith);
}
public class PartRepository : IPartRepository
{
    //implement it calling what ever you need to get the actual data
}

Your parts view wouldn't be able to Select an Item without first request that all items be populated. So lets add in a Action to request all data in the interface. Change IPartsView to be able to request all the data.

public interface IPartsView
{
    IList<Part> Parts { get; set; }

    event EventHandler SelectedItemChanged;
    event Action UserRequestsAllParts;
}

As RubberDuck's post eluded too there is a PartsPresenter. So it probably wouldn't hurt to describe the IPartsPresenter.

public interface IPartsPresenter
{
    IPartsView {get;}
    IPartRepository {get;}
}

Now with all the parts in place it is easy test that the View can request all the data. You can easily select one of those bits of data (any row of your fake data), and perform what you need and want all in memory so you don't have to worry about connection strings, and setting up the tables correctly etc etc. A simple test would look like this.

[TestMethod]
public void WhenUserRequestsAllParts_PartsGetsPopulated()
{
    IPartRepository repository = new MockPartsGenerator();
    var view = new Mock<IPartsView>();
    view.SetupAllProperties();

    var presenter = new PartsPresenter(view.Object, repository); //required change

    //act 
    view.Raise(v => v.UserRequestsAllParts += null, null); //I dont know how mock works here to raise Actions.. sorry

    //assert
    Assert.AreEqual(view.Parts.Count, 2, "MockParsGenerator should have populated View with 2 parts");

    Assert.That(view.Parts[0].Description, Is.EqualTo("Test1"));
}
private class MockPartsGenerator : IPartRepository
{
    //Make the other 3 things throw new NotImplementedException
    public IEnumerable<Part> GetAllParts()
    {
        yield return new Part{ Id = 1, Description="Test1", Rsp = 0x01, Identifier ="IDTENTIFY"}
        yield return new Part{ Id = 2, Description="Test2", Rsp = 0x02, Identifier ="DELETE!"}
    }
}

from here you get the benefit of knowing your presenter populates your view with the correct data when needed. You can independently test the Real IPartRepository to make sure it is pulling data correctly. Then you'll know that it will indeed work as intended

\$\endgroup\$

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.