Whilst upgrading some old code, I've found a situation in which these two OO principles seem to be in conflict with one another.
Consider the following pseudocode (it's a simplified version of what I've encountered):
int numberOfNewRecords;
int numberOfOldRecords;
int numberOfUndefinedRecords;
void ColourAndCount()
{
foreach(Row row in dataGridView1.Rows)
{
switch(row.Cells["Status"])
{
case: "Old"
row.BackColor = Color.Red;
numberOfOldRecords++;
break;
case: "New"
row.BackColor = Color.Blue;
numberOfNewRecords++;
break;
default:
row.BackColor = Color.White;
numberOfUndefinedRecords++;
break;
}
}
}
This code does two things: it tally's up the number of records by their status, and it also colourises each row, again by their status. It's messy, but since these two operations have (thus far) always been called at the same time, it hasn't caused any problems and has made maintenance requirements like additional Statuses easy to add.
Nevertheless, the Single Responsibility Principle tells me I should split this into two separate methods:
(Edit) Minor note: I've just realized I may be misusing the term "Single Responsibility Principle" here, which as I understand it refers to classes. What's the term for the "one operation per method" design pattern?
int numberOfNewRecords;
int numberOfOldRecords;
int numberOfUndefinedRecords;
void Count()
{
foreach(Row row in dataGridView1.Rows)
{
switch(row.Cells["Status"])
{
case: "Old"
numberOfOldRecords++;
break;
case: "New"
numberOfNewRecords++;
break;
default:
numberOfUndefinedRecords++;
break;
}
}
}
void Colour()
{
foreach(Row row in dataGridView1.Rows)
{
switch(row.Cells["Status"])
{
case: "Old"
row.BackColor = Color.Red;
break;
case: "New"
row.BackColor = Color.Blue;
break;
default:
row.BackColor = Color.White;
break;
}
}
}
But this violates Don't Repeat Yourself: the loop and switch statement is duplicated in both methods, and since the most likely upgrade path for this code is the addition of other Statuses, it makes future upgrades more difficult rather than less.
I having trouble working out the most elegant way to refactor this, so I felt it best to ask the community in case there's something obvious I'm missing. How would you address this situation?
(EDIT)
I came up with one possible solution, but it looks to me like an example of over-engineering a simple problem, (and it doesn't really solve the original Single Responsibility problem).
struct Status
{
public string Name,
public int Count,
public Color Colour,
}
Dictionary<string, Status> StatiiDictionary = new Dictionary<string, int>();
void Initialise()
{
StatiiDictionary.Add(new Status("New", 0, Color.Red));
StatiiDictionary.Add(new Status("Old", 0, Color.Blue));
StatiiDictionary.Add(new Status("Undefined", 0, Color.White));
}
void ColourAndCountAllRows()
{
foreach(Row row in dataGridView1.Rows)
{
CountRow(row, StatiiDictionary);
ColourRow(row, StatiiDictionary);
}
}
void CountRow(Row row, Dictionary<string, Status> StatiiDictionary)
{
StatiiDictionary[row.Cells["Status"]].Count++;
}
void ColourRow(Row row, Dictionary<string, Status> StatiiDictionary)
{
row.BackColour = StatiiDictionary[row.Cells["Status"]].Colour;
}
There are lots of programming rules that need to be violated from time to time for pragmatic reasons. In your case, I'd agree that DRY and SRP appear to be competing so I would suggest two criteria to decide the winner:
In this case, the inefficiency of enumerating the rows of the grid twice seems to be the overriding factor to me and DRY would win out in this particular case. In some other case it may be the other way around.
It would be worth adding a comment to explain the decision you made and why so it's clear to anyone looking at the code later. This is exactly the sort of thing that comments should be used for, i.e. why the code is doing what it's doing rather than just what it's doing.
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With