Formatting rows based on priority

I'd like to improve the readability of the following code. The aim of this code is to apply formatting to row(s) in a WinForms DataGridView based on the Priority of an item, determined either by its currentPriorityValue or its dueDate property.

Because I've got tiger striped rows in the grid I used the modulo ternary to get the background to use by default. Then test for the High or Medium priority conditions and set the rowStyles conditionally.

I refactored to use a Tuple because it seemed to tidy the code up bit and let me assign values in my conditional logic, so there was just a single call to FormatRowStyle after the if/else if/else, but in the call to FormatRowStyle I think it no longer clearly expresses what it is doing. Specifically I don't like the if/else block and I don't like the call to FormatRowStyle with the nested Tuple properties showing as Item2.Item1...

Currently this logic is at the UI level. It is a presentational decision but I feel that it should be possible to have the logic handled elsewhere and that would make it easier to test.

```
private void ConditionallyFormatRowsForUrgency(int rowIndex, DateTime dueDate, string currentPriorityValue)
{
var defaultBackColour = rowIndex % 2 == 0 ? VisualSettings.DefaultBackColour : VisualSettings.AlternateBackColour;
var rowStyles = new Tuple>(rowIndex, new Tuple(defaultBackColour, Color.Black, VisualSettings.RegularFont));

if (ToDoUrgency(currentPriorityValue, "High", dueDate, 2))
rowStyles = new Tuple>(rowIndex, VisualSettings.HighPriorityStyle);
else if (ToDoUrgency(currentPriorityValue, "Medium", dueDate, 7))
rowStyles = new Tuple>(rowIndex, VisualSettings.MediumPriorityStyle);

FormatRowStyle(rowStyles.Item1, rowStyles.Item2.Item1, rowStyles.Item2.Item2, rowStyles.Item2.Item3);
}

private static bool ToDoUrgency(string currentPriorityValue, string priorityLevel, DateTime dueDate, int day

Concentrating on the ConditionallyFormatRowsForUrgency method, I see three times rowStyles = new Tuple>(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 accounts for most of the clutter. By specifying an alias (using Style = Tuple at the top) it conveys more meaning and makes it all more readable.

The end result would be like this:

using Style = Tuple;

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(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 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>[] styleDelegates = new Func>[]
{
    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(defaultBackColour, Color.Black, VisualSettings.RegularFont);
        }
};

private void FormatRowForUrgency(int rowIndex, ToDo.Urgency urgencyLevel)
{
    var rowToStyle = dataGridView1.Rows[rowIndex].DefaultCellStyle;

    Tuple 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

StackExchange Code Review Q#11887, answer score: 4