views:

254

answers:

5

I have a business class that contains many properties for various stock-exchange price types. This is a sample of the class:

public class Prices
{
    public decimal Today {get; set;}
    public decimal OneDay {get; set;}
    public decimal SixDay {get; set;}
    public decimal TenDay {get; set;}
    public decimal TwelveDay {get; set;}
    public decimal OneDayAdjusted {get; set;}
    public decimal SixDayAdjusted {get; set;}
    public decimal TenDayAdjusted {get; set;}
    public decimal OneHundredDayAdjusted {get; set;}
}

I have a legacy system that supplies the prices using string ids to identify the price type.

E.g.

Today = "0D"  
OneDay = "1D"  
SixDay = "6D"  
//..., etc.   

Firstly, I load all the values to an IDictionary() collection so we have:

[KEY] VALUE
[0D] => 1.23456
[1D] => 1.23456
[6D] => 1.23456
...., etc.

Secondly, I set the properties of the Prices class using a method that takes the above collection as a parameter like so:

SetPricesValues(IDictionary<string, decimal> pricesDictionary)  
{  
    // TODAY'S PRICE  
    string TODAY = "D0";  
    if (true == pricesDictionary.ContainsKey(TODAY))  
    {  
        this.Today = pricesDictionary[TODAY];  
    }  
    // OneDay PRICE  
    string ONE_DAY = "D1";  
    if (true == pricesDictionary.ContainsKey(ONE_DAY))  
    {  
         this.OneDay = pricesDictionary[ONE_DAY];  
    }  
//..., ..., etc., for each other property   
}  

Is there a more elegant technique to set a large amount of properties? Thanks, j

A: 

I would put the string variables into constants, rather than declare them every time you run the method:

private const string ONE_DAY = "D1";

If you expect the collection parameter to contain all or most of the possible values, then your code is probably cool. If you expect that the dictionary will have a small subset of the possible values, it might be more efficient to use a foreach loop and a switch statement to set values, rather then do a lookup for every possible value every time. It just depends on how many values you need to deal with and how many you get in each method call.

Ray
A: 

The way I see it, you have a few options, depending on your skills, the way you are allowed to change the current POCO's or other classes:

  • If you must use a dictionary, create a similar dictionary which maps the "0D" etc to the OneDay names. Loop through the dictionary and assign using simple reflection.
  • If you can change the way the data is read, have the dictionary read with OneDay etc, instead of the "0D", which is only applicable to the external application.
  • Create an attribute, LegacyKeyAttribute, augment your POCO gettors/settors with this attribute. Now it becomes trivial: loop through the properties of the POCO to find the correct property for your current legacy key.

The last option requires a bit more understanding of C# than many average programmers know: writing and using attributes and reflection. However, in the end it's the cleanest and easiest solution (I'll try to come up with an example).


UPDATE: here's a little example. Meanwhile, many improvement suggestions have been posted, but none still uses attributes, while your case seems ideal. Why? It poses the least burden on existing code, I believe, and it makes reading and understanding your code even easier.

Usage:

// any price:
Prices prices = new Prices();
prices.SetPriceByLegacyName("0D", 1.2345M);

// or, your loop becomes a bit easier:
SetPricesValues(IDictionary<string, decimal> pricesDictionary)  
{  
    foreach(string key in pricesDictionary.Keys)
    {
        // assuming "this" is of type Prices (you didn't specify)
        this.SetPriceByLegacyName(key, pricesDictionary[key]);
    }    
}  

The implementation:

// the simplest attribute class is enough for you:
[AttributeUsage(AttributeTargets.Property)]
public class LegacyNameAttribute : Attribute
{
    public string Name { get; set; }
    public LegacyNameAttribute(string name)
    {
        this.Name = name;
    }
}

// your Prices POCO class becomes easier to read
public class Prices
{
    [LegacyName("0D")]    public decimal Today { get; set; }
    [LegacyName("1D")]    public decimal OneDay { get; set; }
    [LegacyName("6D")]    public decimal SixDay { get; set; }
    [LegacyName("10D")]   public decimal TenDay { get; set; }
    [LegacyName("12D")]   public decimal TwelveDay { get; set; }
    [LegacyName("1DA")]   public decimal OneDayAdjusted { get; set; }
    [LegacyName("6DA")]   public decimal SixDayAdjusted { get; set; }
    [LegacyName("10DA")]  public decimal TenDayAdjusted { get; set; }
    [LegacyName("100DA")] public decimal OneHundredDayAdjusted { get; set; }
}

// an extension method to ease the implementation:
public static class PricesExtensions
{
    public static void SetPriceByLegacyName(this Prices price, string name, decimal value)
    {
        if (price == null)
            throw new ArgumentException("Price cannot be null");

        foreach (PropertyInfo prop in price.GetType().GetProperties())
        {
            LegacyNameAttribute legNameAttribute = (LegacyNameAttribute)
                Attribute.GetCustomAttribute(prop, typeof(LegacyNameAttribute));

            // set the property if the attribute matches
            if (legNameAttribute != null && legNameAttribute.Name == name)
            {
                prop.SetValue(price, value, null);
                break;   // nothing more to do
            }
        }
    }
}

That's all there is to it. Even with all the added lines, it may well be that your total line count becomes less. But more importantly, it becomes easier to maintain and use.

Abel
I'd be afraid that this would be slow due to all the reflection. Any chance of combining this with Aaronaught's dictionary of lambdas?
Gabe
Of course, you can optimize, store the settor and cache it. However, considering that just about every data-to-POCO library uses reflection, this is a good starting point. "All the reflection" is limited: getting all properties at once is the only expensive call, and not even close as expensive as name-based lookup in reflection.
Abel
Also: speed optimization is only useful if the part of the code is slow in comparison to other code. If data is retrieved through XML and then mapped, or through ODBC and then mapped, the relative influence of the method above will be only of the lowest percentile.
Abel
Is it possible to create a dictionary collection of delegates to achieve this? E.g. Dictionary<string, SomeSetterDelegate> PropertySetter PropertySetter.Add("0D", new SetOneDay());
Guazz
@Guazz: yes, that is possible, but not trivial, and likely hardly necessary. To get the settor, add "set_" to the property name and use `GetMethod` instead. Using `Invoke` on a `MethodInfo` (`PropertyInfo` is a MethodInfo) you are only slightly slower than a delegate and it's easier to build a list of MethodInfos. But before you do something complex to achieve something simple, ask yourself the question whether it's worth it. What's the gain? Is it necessary?
Abel
A: 

Just an idea:

interface IPrices_As_String{
 string OD { get; set; }
 // other properties here...
}

interface IPrices{
 decimal Today{get; set;}
}

class Prices : IPrices, IPrices_As_String{
 public decimal Today { get; set; }
 public string IPrices_As_String.OD {
  get { return this.Today.ToString(); }
  set { 
    if(!String.IsNullOrEmpty(value)){
       this.Today = decimal.Parse(value);
    }
  }
 }
}

Then when I am setting the values from the legacy system, I will use the Prices class on the interface as IPrices_As_String like:

IPrices_As_String obj = new Prices();
// set values from the legacy system

IPrices obj2 = obj as IPrices; // will give me the correct object..

.

HTH.

Sunny
I don't think this makes it any easier. You still need at least as much code as his current implementation to set the values, unless you use reflection, in which case you could just use attributes or a mapping array to map between the string keys and fields.
Matti Virkkunen
Sunny
*`string OD { get; set; }`* : you wrote an `O`, not `0`, but look again at the original snippet: these prefixes are numbers. And unfortunately one cannot start a name with a number.
Abel
A: 

Define a dictionary of properties in the constructor e.g.

private Dictionary<int, PropertyInfo> propertyDictionary = new ...

MyClass()
{
    this.propertyDictionary.Add(0, this.GetType().GetProperty("FirstProperty");
    ...
}

then access using an indexed property

decimal this[int index]
{
    get
    {
        PropertyInfo property;
        if (this.propertyDictionary.TryGetValue(index, out property))
        {
            // Not sure I remember the arguments right here:
            property.SetValue(this, new object[] { value });
        }
    set
    {
        // Similar code
    }
}

You could later on improve this code by automatically parsing the properties in the constructor using reflection, adding all properties with an attribute that tells you what the id is. (Instead of adding them manually in the constructor).

Danny Varod
The index can be int/String/Whatever
Danny Varod
+2  A: 

Instead of using a string-to-decimal mapping and checking the dictionary repeatedly, use a delegate mapping/extension method:

public static class PriceConverter
{
    private static readonly Dictionary<string, Action<Prices, decimal>> setters =
        CreateSetterDictionary();

    public static void SetPrice(this Prices p, string id, decimal newPrice)
    {
        Action<Prices, decimal> setter;
        if (setters.TryGetValue(id, out setter))
            setter(p, newPrice);
    }

    private static Dictionary<string, Action<Prices, decimal>>
        CreateSetterDictionary()
    {
        var dic = new Dictionary<string, Action<Prices, decimal>>();
        dic.Add("0D", (p, d) => p.Today = d);
        dic.Add("1D", (p, d) => p.OneDay = d);
        // etc.
        return dic;
    }
}

Then you can write prices.SetPrice("0D", 1.23456).

If you like, add a throw statement at the end of the SetPrice method to handle cases where the id doesn't match anything.

Aaronaught