Concentrating on the ConditionallyFormatRowsForUrgency
method, I see three times rowStyles = new Tuple<int, Tuple<Color, Color, Font>>(rowIndex, ...)
so that is something I would move to a common place. In fact, only its last argument -- lets call it style
-- changes.
The initial value for style
is actually the default for when the two if
-statements don't fire. So I would put that initial value as an else
to that if
-statement.
Last but not least: Tuple<Color, Color, Font>
accounts for most of the clutter. By specifying an alias (using Style = Tuple<Color, Color, Font>
at the top) it conveys more meaning and makes it all more readable.
The end result would be like this:
using Style = Tuple<Color, Color, Font>;
private void ConditionallyFormatRowsForUrgency(int rowIndex, DateTime dueDate, string currentPriorityValue)
{
var defaultBackColour = rowIndex % 2 == 0 ? VisualSettings.DefaultBackColour : VisualSettings.AlternateBackColour;
Style style;
if (ToDoUrgency(currentPriorityValue, "High", dueDate, 2))
style = VisualSettings.HighPriorityStyle;
else if (ToDoUrgency(currentPriorityValue, "Medium", dueDate, 7))
style = VisualSettings.MediumPriorityStyle;
else
style = new Style(defaultBackColour, Color.Black, VisualSettings.RegularFont);
var rowStyles = new Tuple<int, Style>(rowIndex, style);
FormatRowStyle(rowStyles.Item1, rowStyles.Item2.Item1, rowStyles.Item2.Item2, rowStyles.Item2.Item3);
}
You could consider moving defaultBackColor
into the else
as that is the only place it is relevant.
In response to your latest edits: I think that your ToDoUrgency
method is quite short, concise and easy to read. Maybe the length of the if-conditions make you think it is ugly, but there is not much you can do about it.
One important remark: in ConditionallyFormatRowsForUrgency
the style
variable should never be null, or you'll get a NullReferenceException
later on, somewhere else. I therefore strongly recommend that you throw a NotImplementedException
in the switch default
case and remove the = null
from the line Tuple<Color, Color, Font> style = null
. Two reasons: if you ever add a new enum member to Urgency
and you forget to add the appropriate case
to the switch, you'll get an exception right there in your switch instead of in some other method (e.g. FormatRowStyle
). If you then do add the case but forget to set the style
variable to something valid, you'll get a compiler error stating that style
might be used while it has not been assigned a value. This way you ensure that style
is never null and that your switch covers all cases.
Further, on more personal preferences: I'd rename ConditionallyFormatRowsForUrgency
to something like FormatRowForUrgency
. The fact that it is conditional is an implementation detail: you might later on change how the formatting is done and this should not require you to change the name of the method. And it formats only one row, so use singular Row
instead of Rows
. You could also rename ToDoUrgency
to just ToUrgency
as that's what it does.
Lastly, your method ConditionallyFormatRowsForUrgency
does not care about DateTime dueDate
or string currentPriorityValue
, yet is asks for them. Your method cares about Urgency urgency
so that's what I would put in the parameters. This also makes your testing easier, as you can just put a single Urgency
value in the method and see how it works. The conversion from dueDate
and currentPriorityValue
to an Urgency
enumeration member may then be done somewhere else, and tested separately.
private void FormatRowForUrgency(int rowIndex, Urgency urgency)
{ ... }
In regards to the switch-statement: essentially there are four ways to accomplish what you want to do: as a switch-statement, as multiple if-statements, using inheritance, or as an index into a delegate array of functions. Multiple if-statements are probably uglier in this case than a switch-statement, and inheritance would require a complete rewrite of the Urgency enum to a class hierarchy. The index into a delegate array of functions would look like this:
private static readonly Func<int, Tuple<Color, Color, Font>>[] styleDelegates = new Func<int, Tuple<Color, Color, Font>>[]
{
rowIndex => VisualSettings.HighPriorityStyle, // assuming ToDo.Urgency.High = 0
rowIndex => VisualSettings.MediumPriorityStyle, // assuming ToDo.Urgency.Medium = 1
rowIndex => // assuming ToDo.Urgency.Normal = 2
{
var defaultBackColour = rowIndex % 2 == 0 ? VisualSettings.DefaultBackColour : VisualSettings.AlternateBackColour;
return new Tuple<Color, Color, Font>(defaultBackColour, Color.Black, VisualSettings.RegularFont);
}
};
private void FormatRowForUrgency(int rowIndex, ToDo.Urgency urgencyLevel)
{
var rowToStyle = dataGridView1.Rows[rowIndex].DefaultCellStyle;
Tuple<Color, Color, Font> style = styleDelegates[(int)urgencyLevel](rowIndex);
FormatRowStyle(rowToStyle, style);
}
However, I would not recommend it. It is very likely that it severly reduces performance. There is just not much you can do about the switch statement and I would not mind seeing it there where it is now. But an alternative might be to move the switch statement altogether into another method, with the added benefit that it is possible to return
early out of the switch.
private Tuple<Color, Color, Font> GetStyleByUrgency(int rowIndex, ToDo.Urgency urgencyLevel)
{
switch (urgencyLevel)
{
case ToDo.Urgency.High: return VisualSettings.HighPriorityStyle;
case ToDo.Urgency.Medium: return VisualSettings.MediumPriorityStyle;
case ToDo.Urgency.Normal:
var defaultBackColour = rowIndex % 2 == 0 ? VisualSettings.DefaultBackColour : VisualSettings.AlternateBackColour;
return new Tuple<Color, Color, Font>(defaultBackColour, Color.Black, VisualSettings.RegularFont);
default:
throw new InvalidOperationException();
}
}
private void FormatRowForUrgency(int rowIndex, ToDo.Urgency urgencyLevel)
{
var rowToStyle = dataGridView1.Rows[rowIndex].DefaultCellStyle;
Tuple<Color, Color, Font> style = GetStyleByUrgency(rowIndex, urgencyLevel);
FormatRowStyle(rowToStyle, style);
}