3
\$\begingroup\$

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<int, Tuple<Color, Color, Font>>(rowIndex, new Tuple<Color, Color, Font>(defaultBackColour, Color.Black, VisualSettings.RegularFont));

        if (ToDoUrgency(currentPriorityValue, "High", dueDate, 2))
            rowStyles = new Tuple<int, Tuple<Color, Color, Font>>(rowIndex, VisualSettings.HighPriorityStyle);
        else if (ToDoUrgency(currentPriorityValue, "Medium", dueDate, 7))
            rowStyles = new Tuple<int, Tuple<Color, Color, Font>>(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 daysGrace)
    {
        return currentPriorityValue == priorityLevel || dueDate <= DateTime.Now.AddDays(daysGrace);
    }


private void FormatRowStyle(int rowIndex, Color backColour, Color foreColour, Font font)
    {
        dataGridView1.Rows[rowIndex].DefaultCellStyle.BackColor = backColour;
        dataGridView1.Rows[rowIndex].DefaultCellStyle.ForeColor = foreColour;
        dataGridView1.Rows[rowIndex].DefaultCellStyle.Font = font;
    }
\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

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);
}
\$\endgroup\$
1
\$\begingroup\$

In response to your "I still feel there's a better way of handling this than the switch" sometimes when I see switches I think polymorphism and is there a way to handle this via OOP. I typically don't do anything about it but at times it's hinting that I have a problem in my design and I should think a bit more deeper to put the necessary abstractions in place etc

In your case, although this might be needless complexity you could change your enum into a class hierachy of Urgencies. Maybe something like.

   public abstract class Urgency
   {
       public Color ForeColor { get; private set; }
       public Font Font { get; private set; }
       private Color _defaultBackColor { get; }

       protected Urgency(Color color, VisualSettings.RegularFont font, VisualSettings.RegularFont defaultBackColor)
       {
           this.ForeColor = color;
           this.Font = font;
           this._defaultBackColor = defaultBackColor;
       }

       public virtual Color GetBackColor(int rowIndex)
       {
       return this.DefaultBackColor;
       }         

    }

    public class HighUrgency : Urgency
    {
       public HighUrgency(Tuple<Color, Color, Font> priority)
         : this(priority.item2, priority.item3, priority.item1)
      {

      }

      public HighUrgency(Color color, Font font)
        : base(color, font)
     {    
     }
  }

  public class NormalUrgency : Urgency
  {     
     public NormalUrgency(Color color, Font font)
       : base(color, font, VisualSettings.DefaultBackColour)
     {    
     }

  public override Color GetBackColor(int rowIndex)
  {         
     if(rowIndex % 2 == 0) 
     {
        return VisualSettings.AlternateBackColour;
     }
     else
     {
       return base.GetBackColor(rowIndex);
       }
  }

Then I would remove the switch and adjust your priority property to something like:

public virtual Urgency UrgencyLevel
{
    get
    {
        if (Priority.PriorityName == Urgency.High.ToString() || DueDate <= DateTime.Now.AddDays(2))
        {
            return new HighUrgency( VisualSettings.HighPriorityStyle);
        }
        else if (Priority.PriorityName == Urgency.Medium.ToString() || DueDate <= DateTime.Now.AddDays(7))
        {
           return new MediumUrgency( VisualSettings.MediumPriorityStyle);
        }
        else
        {
           // what about passing in VisualSettings.NormalPriorityStyle here??
           return new NormalUrgency(Color.Black, VisualSettings.RegularFont);
        }
   }
} 

Then your method with the switch becomes something like:

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

    cellStyle.BackColor = urgency.GetBackColor(rowIndex);
    cellStyle.ForeColor = urgency.ForeColor;
    cellStyle.Font = urgency.Font;      
}

Adding a new Urgency then becomes a matter of adding a new class and adjusting the if else in the Urgency property.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.