views:

128

answers:

7

Is this good practise? I have 3 DataGridView's and I want to have a facility that allows a user to sort the data by clicking on a column header. I could've had an event handler for the ColumnHeaderMouseClick event for each of these DataGridView's, but instead I made one:

private void dataGridView_ColumnHeaderMouseClick(object sender, DataGridViewCellMouseEventArgs e)
    {
        SortDataGridView((sender as DataGridView), e.ColumnIndex);
    }

SortDataGridView looks like this:

/// <summary>
    /// Sorts a given DataGridView by a column given by its columnIndex.
    /// Default sort (if it isn't currently sorted) is Ascending. If it's 
    /// already sorted Ascending, change it to Descending. If it is Descending,
    /// change it to Ascending.
    /// </summary>
    /// <param name="dataGridViewToSort">The DataGridViewToSort</param>
    /// <param name="columnIndexToSortBy">The index of the column which we want to sort by in the DataGridView.</param>
    private void SortDataGridView(DataGridView dataGridViewToSort, int columnIndexToSortBy)
    {
        switch (dataGridViewToSort.SortOrder)
        {
            case SortOrder.Ascending:
                dataGridViewToSort.Sort(dataGridViewToSort.Columns[columnIndexToSortBy], ListSortDirection.Descending);
                break;
            case SortOrder.Descending:
                dataGridViewToSort.Sort(dataGridViewToSort.Columns[columnIndexToSortBy], ListSortDirection.Ascending);
                break;
            case SortOrder.None:
                dataGridViewToSort.Sort(dataGridViewToSort.Columns[columnIndexToSortBy], ListSortDirection.Ascending);
                break;
            default:
                break;
        }
    }

Each of the DataGridView's ColumnHeaderMouseClick event is hooked up to this handler. This implies that in order to realise which one raised the event at runtime, I have to say (sender as DataGridView). Is this safe? Could sender ever be something that's not a DataGridView?

+1  A: 

If you wanted to be safe you could always check the Type of the sender before attempting to do anything with it.

In theory you shouldn't be setting the handler of any other objects "ColumnHeaderMouseClick" to this so this shouldn't be necessary.

Richard
+3  A: 

I think this is very acceptable and I use this often for controls that have similar functionality. You can add a check in the event to make sure that sender is of the datagrid type if you are concerned with sender being some other object.

Cody C
+5  A: 

not only is it ok, it is better, because it makes your code more reuseable, maintainable and extensible.

Charles Bretana
A: 

As long as you write the handler to properly handle each control... which is best when the exact same thing needs to be done on multiple controls.

Basically as long as it works, and will be easy to maintain you should be good.

Telos
A: 

It's fine and safe. A small correction: you would check for which one raised the event by

if (sender == dataGrid1)

etc. But if you have specific functionality that requires knowing which one raised the event then you should probably refactor to individual handlers.

Jamie Ide
A: 

This solution is absolutely fine in my opinion. You saved two event handlers, that would actually do exactly the same stuff. If there were different thins to do in each of the handlers, you shouldn't have done it that way, but here it's saving development and maintaining time.

As soon as one of the DataGridViews has a specific job to do on the event handler you should give it an Event Handler for it's own.

bbohac
+2  A: 

it's good practice and follows the DRY ( dont' repeat yourself ) principle

Kumar