Better method for my property Checking, replacement of switch statement

Question

I'm hoping this is the correct place for this, I'm struggling a little for wording as i'm not sure what you would call this.

Basically, i have a system in place where my datagrid marks cells that have changed with a new background colour, to do this i have a method in the object that contains these properties that receives a string which is the name of the property to check, and then a switch statement that takes that string to check the correct property.

public Color HasChanged(string value)
    {
        switch (value)
        {
            case "CILRef":
                if (_localShipment.cilRef != _originalShipment.cilRef)
                {
                    return Colors.SkyBlue;
                }
                else
                {
                    return Colors.White;
                }
            case "ArrivedAtPortDate":
                if (_localShipment.arrivedAtPortDate != _originalShipment.arrivedAtPortDate)
                {
                    return Colors.SkyBlue;
                }
                else
                {
                    return Colors.White;
                }
  }

I've removed the rest of the properties for brevity.

Now i get the nagging sensation that there is a cleaner way to do this string>property without using a switch statement, but i can't for the life of me find anything on google, it's hard to search without some keyword to go on.

I'm also attempting to only save those properties that have changed, i was going to place any changed property name into an array, and then have a loop with yet another switch statement that checked that array and then saved the correct property. However this again seems untidy to me.

is there a cleaner solution to this, hopefully that could handle the addition of new properties without needing to add new code to the switch statements.

I can include the rest of the code that does this checking (namely the WPF binding on the datagrid, and a converter that calls the checking method with the property name as a string parameter) if needed.

EDIT:

To show the rest of my code, hopefully explaining a few things.

I have a datagrid in xaml that contains these properties. below is an example:

<DataGridTemplateColumn Header="CILRef">
                    <DataGridTemplateColumn.CellTemplate>
                        <DataTemplate>
                            <TextBlock Text="{Binding CILRef}"> 
                                <TextBlock.Background> 
                                    <SolidColorBrush Color="{Binding Converter={StaticResource hasChangedConverter}, ConverterParameter='CILRef'}"/> 
                                </TextBlock.Background>
                            </TextBlock>
                        </DataTemplate>
                    </DataGridTemplateColumn.CellTemplate>
                    <DataGridTemplateColumn.CellEditingTemplate>
                        <DataTemplate>
                            <TextBox Text="{Binding CILRef, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}"/>
                        </DataTemplate> 
                    </DataGridTemplateColumn.CellEditingTemplate>
                </DataGridTemplateColumn>

as you can see, by this line : <SolidColorBrush Color="{Binding Converter={StaticResource hasChangedConverter}, ConverterParameter='CILRef'}"/>

The background is bound using a converter, which is:

class HasChangedConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        try
        {
            var shipment = value as Shipment;
            var property = parameter as string;

            return shipment.HasChanged(property);
        }
        catch (Exception ex)
        {
            return Colors.HotPink;
        }
    }

    public object ConvertBack(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        throw new Exception("The method or operation is not implemented.");
    }
}

Hopefully this will explain how the code works better than i've been doing in the comments.

Answer 1

Ok, so having spoken to others on stackoverflow, i have change the code, this (i hope) satisfies all the issue's I've pointed out and all the issues pointed out to me.

The HasChanged is now:

public Color HasChanged(string value)
{
    try
    {
        var data1 = _localShipment.GetType().GetProperty(value, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance).GetValue(_localShipment, null);
        var data2 = _originalShipment.GetType().GetProperty(value, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance).GetValue(_originalShipment, null);
        return data1 != data2 ? Colors.SkyBlue : Colors.White;
    }
    catch (Exception ex)
    {
        return Colors.White;
    } 
}

EDIT:

This has been changed to the following methods:

 private object GetPropValue(object src, string propName)
    {
            PropertyInfo p = src.GetType().GetProperty(propName, BindingFlags.IgnoreCase | BindingFlags.Public | BindingFlags.Instance);
            string value = (string)p.GetValue(src, null);
            return value;
    }

    public bool HasChanged(string value)
    {
        var data1 = GetPropValue(_localShipment, value);
        var data2 = GetPropValue(_originalShipment, value);
        return data1 != data2 ? true : false;
    }
    #endregion
}

and the converter has been changed to :

class HasChangedConverter : IValueConverter
{
    public object Convert(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        if (value == null) { return Colors.White; }
        var shipment = value as Shipment;
        var property = parameter as string;
        if (shipment.HasChanged(property)) { return Colors.SkyBlue; } else { return Colors.White; }
    }

    public object ConvertBack(object value, Type targetType, object parameter, System.Globalization.CultureInfo culture)
    {
        throw new Exception("The method or operation is not implemented.");
    }
}

This solves the issues mentioned previously in this question.

Answer 2

1. As you already discovered, if you want to access a member based on a string name, use reflection.
2. You shouldn't mix business logic with presentation: HasChanged shouldn't return a Color, it should return a bool. The colors should the be defined separately.
3. Use as only when you expect to get the wrong type and in that case, always test for null. If you're not going to do that, just use a cast.
4. I don't think that indicating a general exception using a color is a good idea, because it hides all information about what specific exception occurred. If you're expecting only some specific exception, then catch only that.

Answer 3

Well it seems to me you have a key and a chunk of code that returns a colour based on some boolean check.

I would probably suggest a Function collection.

e.g

Dictionary<string,Func<Color>> Actions;

void InitActions()
{
  Actions = new Dictionary<string,Func<Color>>();


  Actions.add("CILRef",()=>
{
  return _localShipment.cilRef != _originalShipment.cilRef ? Colors.SkyBlue : Color.White;
});

} 

and to use it:

public Color HasChanged(string value)
{
  Color highlightColor = Color.White;
  if(Actions.Contains(value))
    highlightColor = Actions[value];

return highlightColor;
}

Although that doesn't really solve your problem.

I usually like to answer a persons question first in case they choose not to do a large refactor but I would take a closer look at what you are trying to do.

You have a number of larger design problems:

• You have a lot of repetition.
• Next nested loops/logic are usually good indications of requiring a refactor.
• You have "Magic Strings", what if one of those is misspelt, will your whole application break?
• You shouldn't be tying application logic directly to the ui in the first place.

Ideally I imagine you want something more akin to:

void DataGridSelectionChanged(object sender,EventArgs e)
{
   var grid = sender as Grid;   
   if(grid == null) return;

   string selectedValue = grid.SelectedValue ?? DefaultValue;

   Color highlightColor = GetHighlightColour(selectedValue);

   ...
}

In short the prospect of setting the highlight colour should not really be dependent on the raw values, and if indeed it has to be it should be done in a safe manner. as it is there are a number of way's to break your application some accidentally while developing others while using the application by passing invalid data.