views:

248

answers:

8

I have an object that needs to be serialized to an EDI format. For this example we'll say it's a car. A car might not be the best example b/c options change over time, but for the real object the Enums will never change.

I have many Enums like the following with custom attributes applied.

public enum RoofStyle
{
    [DisplayText("Glass Top")]
    [StringValue("GTR")]
    Glass,
    [DisplayText("Convertible Soft Top")]
    [StringValue("CST")]
    ConvertibleSoft,
    [DisplayText("Hard Top")]
    [StringValue("HT ")]
    HardTop,
    [DisplayText("Targa Top")]
    [StringValue("TT ")]
    Targa,
}

The Attributes are accessed via Extension methods:

public static string GetStringValue(this Enum value)
{
    // Get the type
    Type type = value.GetType();

    // Get fieldinfo for this type
    FieldInfo fieldInfo = type.GetField(value.ToString());

    // Get the stringvalue attributes
    StringValueAttribute[] attribs = fieldInfo.GetCustomAttributes(
        typeof(StringValueAttribute), false) as StringValueAttribute[];

    // Return the first if there was a match.
    return attribs.Length > 0 ? attribs[0].StringValue : null;
}

public static string GetDisplayText(this Enum value)
{
    // Get the type
    Type type = value.GetType();

    // Get fieldinfo for this type
    FieldInfo fieldInfo = type.GetField(value.ToString());

    // Get the DisplayText attributes
    DisplayTextAttribute[] attribs = fieldInfo.GetCustomAttributes(
        typeof(DisplayTextAttribute), false) as DisplayTextAttribute[];

    // Return the first if there was a match.
    return attribs.Length > 0 ? attribs[0].DisplayText : value.ToString();
}

There is a custom EDI serializer that serializes based on the StringValue attributes like so:

    StringBuilder sb = new StringBuilder();
    sb.Append(car.RoofStyle.GetStringValue());
    sb.Append(car.TireSize.GetStringValue());
    sb.Append(car.Model.GetStringValue());
    ...

There is another method that can get Enum Value from StringValue for Deserialization:

   car.RoofStyle = Enums.GetCode<RoofStyle>(EDIString.Substring(4, 3))

Defined as:

public static class Enums
    {
        public static T GetCode<T>(string value)
        {
            foreach (object o in System.Enum.GetValues(typeof(T)))
            {
                if (((Enum)o).GetStringValue() == value.ToUpper())
                    return (T)o;
            }
            throw new ArgumentException("No code exists for type " + typeof(T).ToString() + " corresponding to value of " + value);
        }
} 

And Finally, for the UI, the GetDisplayText() is used to show the user friendly text.

What do you think? Overkill? Is there a better way? or Goldie Locks (just right)?

Just want to get feedback before I intergrate it into my personal framework permanently. Thanks.

+1  A: 

I don't see a problem with it - actually, I do the same. By this, I achieve verbosity with the enum, and can define how the enum is to be translated when I use it to request data, eg. RequestTarget.Character will result in "char".

Femaref
+3  A: 

Personally, I think you are abusing the language and trying to use enums in a way they were never intended. I would create a static class RoofStyle, and create a simple struct RoofType, and use an instance for each of your enum values.

mikerobi
Enum values have to be primitive types. You need to clarify what it is that you're trying to say here, because as written it makes no sense.
Aaronaught
@Aaronaught, I thought I was pretty clear, if you wan't this type of behavior don't use enums. Enums map identifiers to integers; From my perspective, the OP is essentially attaching string attributes to an integer type. Making an int behave as storage container for a string is a bad practice.
mikerobi
Aaronaught
@Aaronaught, It is pretty easy "RoofStyle.HardTop.DisplayText". I don't see anything at that page that suggests this is a recommended approach. Maybe it is, maybe it isn't; its just not an approach I would take, lets just agree to disagree.
mikerobi
Mike, that doesn't create a map, it just puts the data in a slightly different place, one that's hard to serialize and impossible to constrain (the latter being the primary purpose of an enum). If this is a performance concern then that is easily solved by generating a reverse lookup table; otherwise, I fail to see how this is "abusing" the language, it's incredibly common to annotate enums this way.
Aaronaught
@Aaronaught: You can constrain the enumeration class, just seal it and use a private constructor. Also, I think what @mikerobi is saying is that enumerations are useful and so are annotations, but enumerations + annotations is a less-than-ideal way to represent a known set of objects which have multiple pieces of data. Classes are *intended* to represent objects with multiple pieces of data; attaching that data in another fashion is neat, certainly, but you can't really say that the reflection code is easier and more maintainable than a simple enumeration class.
Bryan Watts
@Bryan: Sorry but that makes no sense. You don't "constrain" a class, you *constrain* the possible values that a variable or argument can be, and using a class or struct instead of an enumeration doesn't give you that. OK, you can make the constructor private, and have a bunch of `static readonly` instances, and so on and so forth, but that's not exactly the same thing, and if you actually try to hash this out you'll quickly see that the reflection-based system is *way* easier to work with. I think that reflection just scares some people and *anything* else is always better.
Aaronaught
@Aaronaught: I love reflection, so that's not it. The point I am trying to make is that enumerations are a *pattern*. One specific form of enumeration is the named integer, so special in fact that C# made it a first-class citizen. However, enumeration classes are just as legitimate a form of the pattern. Constraining in this context simply means limiting the available values; the *how* is an implementation detail (compiler or private constructor). I have written both forms of this system and much prefer the enumeration classes: they represent exactly what I want, without more complexity.
Bryan Watts
@Aaronaught: Also, thanks for assuming that I fear reflection and have never hashed out these ideas. Neither could be further from the truth. I speak from a deep well of experience and my points can't be dismissed ad hominem.
Bryan Watts
@Bryan: In that case, I think you should submit an answer with a realistic implementation. If it's genuinely simpler/shorter/more maintainable than the annotation-based version, I'll upvote you. What I disapprove of is vague sentiments to the effect of "my solution is better than yours but I'm too busy/lazy/important to spell it out."
Aaronaught
@Aaronaught: Posted. Feel free to take the conversation there and away from poor @mikerobi's answer.
Bryan Watts
+1  A: 

IMHO, the design is solid, and will work. However, reflection tends to be a litle slow, so if those methods are used in tight loops, it might slow down the whole application.

You could try caching the the return values into a Dictionary<RoofStyle, string> so they are only reflected once, and then fetched from cache.

Something like this:

    private static Dictionary<Enum, string> stringValues 
      = new Dictionary<Enum,string>();

    public static string GetStringValue(this Enum value)
    {
        if (!stringValues.ContainsKey(value))
        {
            Type type = value.GetType();
            FieldInfo fieldInfo = type.GetField(value.ToString());
            StringValueAttribute[] attribs = fieldInfo.GetCustomAttributes(
                typeof(StringValueAttribute), false) as StringValueAttribute[];
            stringValues.Add(value, attribs.Length > 0 ? attribs[0].StringValue : null);
        }
        return stringValues[value];
    }
SWeko
Wouldn't the Dictionary Cache defeat the purpose of the custom attributes? I would have to make multiple dictionaries to hold string values, display text, etc. I know in 4.0 I could cache a Tuple, which would definitely provide a good caching solution. I might integrate that when we move to 4.0. Thanks
CkH
edited the response to include an example of what I meant.
SWeko
+6  A: 

The part you're using to serialize is fine. The deserialization part is awkwardly written. The main problem is that you're using ToUpper() to compare strings, which is easily broken (think globalization). Such comparisons should be done with string.Compare instead, or the string.Equals overload that takes a StringComparison.

The other thing is that performing these lookups again and again during deserialization is going to pretty slow. If you're serializing a lot of data, this could actually be quite noticeable. In that case, you'd want to build a map from the StringValue to the enum itself - throw it into a static Dictionary<string, RoofStyle> and use it as a lookup for the round-trip. In other words:

public static class Enums
{
    private static Dictionary<string, RoofStyle> roofStyles =
        new Dictionary<string, RoofStyle>()
    {
        { "GTR", RoofStyle.Glass },
        { "CST", RoofStyle.ConvertibleSoft },
        { "HT ", RoofStyle.HardTop },
        { "TT ", RoofStyle.TargaTop }
    }

    public static RoofStyle GetRoofStyle(string code)
    {
        RoofStyle result;
        if (roofStyles.TryGetValue(code, out result))
            return result;
        throw new ArgumentException(...);
    }
}

It's not as "generic" but it's way more efficient. If you don't like the duplication of string values then extract the codes as constants in a separate class.

If you really need it to be totally generic and work for any enum, you can always lazy-load the dictionary of values (generate it using the extension methods you've written) the first time a conversion is done. It's very simple to do that:

static Dictionary<string, T> CreateEnumLookup<T>()
{
    return Enum.GetValues(typeof(T)).ToDictionary(o => ((Enum)o).GetStringValue(),
        o => (T)o);
}

P.S. Minor detail but you might want to consider using Attribute.GetCustomAttribute instead of MemberInfo.GetCustomAttributes if you only expect there to be one attribute. There's no reason for all the array fiddling when you only need one item.

Aaronaught
Great point on the string.Compare, I'll integrate. As for the Enum.Parse, that would work if I were using the Enum values, however the values here are the StringValues, "GTR", "TT ", Etc.
CkH
@CkH: Yeah, I recognized that and deleted it. I think you're really better off using lookups instead, generating them at compile-time or lazy-loading at runtime.
Aaronaught
+1  A: 

Can't say I've ever seen it done that way but the consumer code is relatively simple so I'd probably enjoy using it.

The only thing that sticks out for me is the potential for consumers having to deal with nulls - which might be able to be removed. If you have control over the attributes (which you do, from the sounds of it), then there should never be a case where GetDisplayText or GetStringValue return null so you can remove

return attribs.Length > 0 ? attribs[0].StringValue : null;

and replace it with

return attribs[0].StringValue;

in order to simplify the interface for consumer code.

SnOrfus
The only reason this is there, is b/c these extension methods extend all Enums. Therefore if one were to use an enum that does not have these custom attributes on them, it would throw an exception rather than returning null, which is desired.
CkH
wouldn't a better option be (for both String and Display) to return the value of the calling enum instead of null?
SWeko
Good point, that would be a good approach.
CkH
+2  A: 

Why don't you create a type with static members such as mikerobi said

Example...

public class RoofStyle
{
    private RoofStyle() { }
    public string Display { get; private set; }
    public string Value { get; private set; }

    public readonly static RoofStyle Glass = new RoofStyle
    {
        Display = "Glass Top",  Value = "GTR",
    };
    public readonly static RoofStyle ConvertibleSoft = new RoofStyle
    {
        Display = "Convertible Soft Top", Value = "CST",
    };
    public readonly static RoofStyle HardTop = new RoofStyle
    {
        Display = "Hard Top", Value = "HT ",
    };
    public readonly static RoofStyle Targa = new RoofStyle
    {
        Display = "Targa Top", Value = "TT ",
    };
}

BTW...

When compiled into IL an Enum is very similar to this class structure.

... Enum backing fields ...

.field public specialname rtspecialname int32 value__
.field public static literal valuetype A.ERoofStyle Glass = int32(0x00)
.field public static literal valuetype A.ERoofStyle ConvertibleSoft = int32(0x01)
.field public static literal valuetype A.ERoofStyle HardTop = int32(0x02)
.field public static literal valuetype A.ERoofStyle Targa = int32(0x03)

... Class backing fields ...

.field public static initonly class A.RoofStyle Glass
.field public static initonly class A.RoofStyle ConvertibleSoft
.field public static initonly class A.RoofStyle HardTop
.field public static initonly class A.RoofStyle Targa
Matthew Whited
Interesting approach, however, this would be useless if you were to serialize the Car object with the DataContractSerializer. DataMember is ignored on any Static Member. Thus you could never set the roofstyle on the client. Enums work much better. I need objects that can be used in WCF. The Extensions and attributes won't serialize but they are only for server side use anyway.
CkH
The member of Car would not be static.
Matthew Whited
Understandable, however all of the properties of RoofStyle are static, therefore on the client, you would get RoofStyle class with no members. The client would have no idea what the RoofStyles are.
CkH
You need to check that again. Display and Value are not static. Only the "enumeration" style values are static.
Matthew Whited
BTW. You might want to override `GetHashCode()` and `Equals(...)` so deserialized instances would be equal to the static values.
Matthew Whited
Perhaps, I'm not describing this clearly. I agree with you that Display and Value would show up on the client. What I'm saying is that with an enum, the client would be able to select an value (Glass, HardTop, etc.). With your RoofStyle Class, the client would have no idea what the options would be. It actually serializes like so:<xs:complexType name="RoofStyle"> <xs:sequence> <xs:element minOccurs="0" name="Display" nillable="true" type="xs:string" /> <xs:element minOccurs="0" name="Value" nillable="true" type="xs:string" /> </xs:sequence> </xs:complexType>
CkH
Notice that Glass, ConvertibleSoft, HardTop, Targa aren't serialized. Therefore the client would not know what the possible options are.
CkH
It's probably more of the fact they have private setters. I would probably just store a single value and lookup the particular RoofStyle each time. To me it really depends on how often you are looking up the data and if you want to use a lookup table or reflection. And the XSD enumeration are up to you on how you want to implment them. That is a different issue then how the data would serialize.
Matthew Whited
+1  A: 

Here is a base class I use for enumeration classes:

public abstract class Enumeration<T, TId> : IEquatable<T> where T : Enumeration<T, TId>
{
    public static bool operator ==(Enumeration<T, TId> x, T y)
    {
        return Object.ReferenceEquals(x, y) || (!Object.ReferenceEquals(x, null) && x.Equals(y));
    }

    public static bool operator !=(Enumeration<T, TId> first, T second)
    {
        return !(first == second);
    }

    public static T FromId(TId id)
    {
        return AllValues.Where(value => value.Id.Equals(id)).FirstOrDefault();
    }

    public static readonly ReadOnlyCollection<T> AllValues = FindValues();

    private static ReadOnlyCollection<T> FindValues()
    {
        var values =
            (from staticField in typeof(T).GetFields(BindingFlags.Static | BindingFlags.Public)
            where staticField.FieldType == typeof(T)
            select (T) staticField.GetValue(null))
            .ToList()
            .AsReadOnly();

        var duplicateIds =
            (from value in values
            group value by value.Id into valuesById
            where valuesById.Skip(1).Any()
            select valuesById.Key)
            .Take(1)
            .ToList();

        if(duplicateIds.Count > 0)
        {
            throw new DuplicateEnumerationIdException("Duplicate ID: " + duplicateIds.Single());
        }

        return values;
    }

    protected Enumeration(TId id, string name)
    {
        Contract.Requires(((object) id) != null);
        Contract.Requires(!String.IsNullOrEmpty(name));

        this.Id = id;
        this.Name = name;
    }

    protected Enumeration()
    {}

    public override bool Equals(object obj)
    {
        return Equals(obj as T);
    }

    public override int GetHashCode()
    {
        return this.Id.GetHashCode();
    }

    public override string ToString()
    {
        return this.Name;
    }

    #region IEquatable

    public virtual bool Equals(T other)
    {
        return other != null && this.IdComparer.Equals(this.Id, other.Id);
    }
    #endregion

    public virtual TId Id { get; private set; }

    public virtual string Name { get; private set; }

    protected virtual IEqualityComparer<TId> IdComparer
    {
        get { return EqualityComparer<TId>.Default; }
    }
}

An implementation would look like:

public sealed class RoofStyle : Enumeration<RoofStyle, int>
{
    public static readonly RoofStyle Glass = new RoofStyle(0, "Glass Top", "GTR");
    public static readonly RoofStyle ConvertibleSoft = new RoofStyle(1, "Convertible Soft Top", "CST");
    public static readonly RoofStyle HardTop = new RoofStyle(2, "Hard Top", "HT ");
    public static readonly RoofStyle Targa = new RoofStyle(3, "Targa Top", "TT ");

    public static RoofStyle FromStringValue(string stringValue)
    {
        return AllValues.FirstOrDefault(value => value.StringValue == stringValue);
    }

    private RoofStyle(int id, string name, string stringValue) : base(id, name)
    {
        StringValue = stringValue;
    }

    public string StringValue { get; private set; }
}

You would use it during serialization like this:

var builder = new StringBuilder();

builder.Append(car.RoofStyle.StringValue);
...

To deserialize:

car.RoofStyle = RoofStyle.FromStringValue(EDIString.Substring(4, 3));
Bryan Watts
Okay, this is equivalent in functionality to the first code snippet in the question (the enumeration with decorated members). What about the hard part though - serializing it and mapping to and from the different member types?
Aaronaught
@Aaronaught: I went ahead and posted a class of mine which makes enumeration classes easier to write. I use it mostly with NHibernate, but the model serves fine for any enumeration.
Bryan Watts
Fair enough. Looks about the same to me in terms of complexity, but it's certainly a valid alternative. +1.
Aaronaught
@Aaronaught: The only significant difference I see is the lack of custom attributes. Because of that, it can be argued that adding new values alongside `StringValue` is less complex. Also, in the attributed solution, the consumer is responsible for harboring the cache. Not a major downside, but it does make the system less self-contained. Thanks for the lively discussion.
Bryan Watts
+1  A: 

I know this question has already been answered, but while ago I posted the following code fragment on my personal blog, which demonstrates faking Java style enums using extension methods. You might find this method works for you, especially as it overcomes the overhead of accessing Attributes via reflection.

using System;
using System.Collections.Generic;

namespace ScratchPad
{
    internal class Program
    {
        private static void Main(string[] args)
        {
            var p = new Program();
            p.Run();
        }

    private void Run()
    {
        double earthWeight = 175;
        double mass = earthWeight / Planet.Earth.SurfaceGravity();

        foreach (Planet planet in Enum.GetValues(typeof(Planet))) {
            Console.WriteLine("Your weight on {0} is {1}", planet, planet.SurfaceWeight(mass));
        }
    }
}

public enum Planet
{
    Mercury,
    Venus,
    Earth,
    Mars,
    Jupiter,
    Saturn,
    Uranus,
    Neptune
}

public static class PlanetExtensions
{
    private static readonly Dictionary<Planet, PlanetData> planetMap = new Dictionary<Planet, PlanetData>
      {
          {Planet.Mercury, new PlanetData(3.303e+23, 2.4397e6)},
          {Planet.Venus, new PlanetData(4.869e+24, 6.0518e6)},
          {Planet.Earth, new PlanetData(5.976e+24, 6.37814e6)},
          {Planet.Mars, new PlanetData(6.421e+23, 3.3972e6)},
          {Planet.Jupiter, new PlanetData(1.9e+27,   7.1492e7)},
          {Planet.Saturn, new PlanetData(5.688e+26, 6.0268e7)},
          {Planet.Uranus, new PlanetData(8.686e+25, 2.5559e7)},
          {Planet.Neptune, new PlanetData(1.024e+26, 2.4746e7)}
      };

    private const double G = 6.67300E-11;

    public static double Mass(this Planet planet)
    {
        return GetPlanetData(planet).Mass;
    }

    public static double Radius(this Planet planet)
    {
        return GetPlanetData(planet).Radius;
    }

    public static double SurfaceGravity(this Planet planet)
    {
        PlanetData planetData = GetPlanetData(planet);

        return G * planetData.Mass / (planetData.Radius * planetData.Radius);
    }

    public static double SurfaceWeight(this Planet planet, double mass)
    {
        return mass * SurfaceGravity(planet);
    }

    private static PlanetData GetPlanetData(Planet planet)
    {
        if (!planetMap.ContainsKey(planet))
            throw new ArgumentOutOfRangeException("planet", "Unknown Planet");

        return planetMap[planet];
    }

    #region Nested type: PlanetData

    public class PlanetData
    {            
        public PlanetData(double mass, double radius)
        {
            Mass = mass;
            Radius = radius;
        }

        public double Mass { get; private set; }
        public double Radius { get; private set; }
    }

    #endregion
    }
}
Ed Courtenay
I like this approach, however it's not generic enough for my needs. I would have to write a separate extension class for every Enum I have and there are a lot. I definitely have a use for this approach and will use it when it's applicable. Thanks for sharing, very clean and excellent use for extension methods.
CkH