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.

• apieceoffruit: That doesn't really change much from the points I mentioned below although as Vogel612 said , you shouldn't use exceptions in expected areas. you should return the Pink value if the variable is null, not do the try block, they are slow and bad practice in logic. Binding is fine but if the values are always the same and they match certain colours...pair them. have a collection of matching enum/colours to values, I would use a dictionary, then your converter can just pull out the right value. Converters are meant to be light weight way's to visualize data differently,not heavy business logic.
• Ben: I'm still not sure what you mean by this, the design of this is if the value has changed from the original loaded value, it displays skyblue. the colour is simply a representation of data that is dirty.
• Vogel612: @user1412240 please include the full switch-statement in your question. also: Why not do the job with simple JavaScript? onChange="this.style.add('background-color', '#lightblueRGB');"
• Ben: if you look at my own answer below, i have removed the switch statement completely now. also, i was not aware you could use javascript in a WPF application, also that would not be a "dirty" flag just a changed flag, which is not what was requested.
• Vogel612: @user1412240 sorry, I'm not experienced with WPF, but I can tell that the difference between a dirty- and changed-flag is only one if-statement.

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.

• Max: This is basically what I was going to post haha. It looks good except for in your catch block you should instead handle the exception. (Unless of course you just want to consume the exception and return a default color...)
• Magus: @Max: The part of your comment in parentheses is wrong, but only because the type of the exception is Exception. If there's a StackOverflowException or an OutOfMemoryException, trying to handle it is a very bad idea. No one wants that. This is more for the questioner's benefit than yours, though, since I assume you already know that.
• Ben: See the edits above, this has been changed in an attempt to solve the issues people have pointed out.

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.

• Ben: Thanks for this. Much clearer than trying to understand via comments. I've already changed the gas change to return a boil and the converter handles the color. The try catch has been removed. Quick question tho. I've always been under the assumption that using as was faster and cleaner than a cast. That comes from uni and is most likely wrong
• svick: Using as and a null check is going to be faster than cast and catch for NullReferenceException, that's probably where the speed argument comes from. But if you're not going to do that, just use a cast. And I think that clearer code is the one that more accurately describes your intentions. And here, that's a cast, not as.
• Ben: Ok, great reply, sorry about the big typos in the original comment, i was attempting to reply via phone for the first time. and it clearly did not go well.

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.

• Ben: I appreciate your comments, This system was requested by the designer, it highlights changes to the datagrid that will be saved in the next "save all", these changes must be data based (different from the loaded data). Also the default on the switch case throws an ArgumentOutOfRangeException. I hope this clears up some of your concerns, and explains why I've chosen to do it this way.
• Vogel612: @user1412240 one should not use Exceptions for "expected" errors. Actually I do that myself more often than I want to admit, but you should instead return a meaningful errorvalue. ;)#
• Ben: Just to note, the string value that is passed is hard coded, not a parameter that is set by the user. It should only ever error if i've made a mistake during development, never during runtime.
• apieceoffruit: @user1412240 which goes back to my original point, why use raw strings that could cause a mistake in the first place? If they are hard coded and never change replace them for const strings and refer to them instead.
• Ben: I think it would help if i provided the rest of the code, perhaps that will explain better than i can via comments. Please see my edit (in a few mins)