Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following code to load an enumeration into a bound combo box, but am not quite happy with it.

I would prefer to store the actual enum value in the combo box and bind directly to it. However, I can't quite figure out how to do so.

 public enum HemEnum
    {
        HemNone = -1,
        Hemsew = 0,
        HemWeld = 1,
        Hemdoublefold = 2
    }


 public static void LoadHemCombo(ComboBox cbo)
    {
    var values = from Enum e in Enum.GetValues(typeof(HemEnum))
                     select new { ID = e, Name = e.ToString() };

          foreach (var value in values)
    {
        var s = GetHemTypeDescription((HemEnum)value.ID );
        cbo.Items.Add(s);
    }
    }

  public static string GetHemTypeDescription(HemEnum hemType)
    {
        string s = null;
        switch (hemType)
        {
            case HemEnum.HemNone:
                s = "none";
                break;
            case HemEnum.Hemsew:
                s = "sewn";
                break;
            case HemEnum.HemWeld:
                s = "welded";
                break;
            case HemEnum.hemdoublefold:
                s = "double folded";
                break;
            default:
                s = "not known";
                break;
        }
        return s;
    }

Inside the form load event I load the combo and bind to it.

 LoadHemCombo(cboHem)
 cboHem.DataBindings.Add("Text", myBindingSource, "HemTypeDescription");

 class myObject
 {
    public string HemTypeDescription
    {
        get
        {
            return GetHemTypeDescription(this.HemType);

        }
        set
        {
            this.HemType = GetHemTypeFromDescription(value);
        }
  }

public static vHemEnum GetHemTypeFromDescription(string description)
    {
        int r =0;
        for (var i = (int)HemEnum.HemNone; i <= (int)HemEnum.Hemdoublefold; i++)
        {
            var s = GetHemTypeDescription((HemEnum)i);
            if (description == s)
            {
                r  = i;
                break;
            }
        }
        return (HemEnum)r;
    }
 }

in the designer I set myBindingSource.DataSource = myObject

and before loading the form I create an instance of myObject and add it to myBindingSource using

myBindingSource.Add(myObject)
share|improve this question
2  
This looks like it's winforms. It's not wpf, is it? If it's WPF, you've missed something. If it's WinForms, you're missing out ...please edit tags accordingly :) –  Mat's Mug Jan 13 at 18:03
    
Thanks. It is winforms, I fixed the tag –  kirsten g Jan 13 at 18:06
    
I've got a custom class devoted to solving this over here. All the advice below is good advice, but this class also addresses pulling the data back out as the right type. –  Bobson Jan 13 at 22:32
    
Thanks every one. I also made it generic. So now I just have 'public void LoadCombo<T>(ComboBox cbo)' –  kirsten g Jan 13 at 23:31

3 Answers 3

up vote 6 down vote accepted

Here may be an decent way of doing what you want:

public enum HemEnum
{
    [Description("none")]
    HemNone = -1,
    [Description("sewn")]
    Hemsew = 0,
    [Description("welded")]
    HemWeld = 1,
    [Description("double folded")]
    Hemdoublefold = 2
}


public static void LoadHemCombo(ComboBox cbo)
{
    cbo.DataSource = Enum.GetValues(typeof(HemEnum))
        .Cast<Enum>()
        .Select(value => new
        {
            (Attribute.GetCustomAttribute(value.GetType().GetField(value.ToString()), typeof(DescriptionAttribute)) as DescriptionAttribute).Description,
            value
        })
        .OrderBy(item => item.value)
        .ToList();
    cbo.DisplayMember = "Description";
    cbo.ValueMember = "value";
}

Do note, this method requires a [Description] on every enum member. If one isn't there, you'll get a NullReferenceException. This requirement keeps the code simple by not needing a nullity check.

share|improve this answer
    
This is very similar to what I use, but I've extended it a bit further so that the [Description] attribute doesn't have to be there. See the code here for an example. –  Bobson Jan 13 at 22:27
    
@Bobson I just commented on your code bits with one of my own. –  Jesse C. Slicer Jan 13 at 22:55
    
Sorry, I phrased that rather misleadingly (too much editing). My code is a slight modification of the linked code, which itself supported missing [Description] attributes. I didn't intend to take credit for writing the initial EnumLabel that I linked. I do like your modifications, though. –  Bobson Jan 13 at 23:00
1  
Cool, heh. I hope whomever wrote the linked code doesn't take any offense. :) –  Jesse C. Slicer Jan 13 at 23:07
    
Wonderful. I changed the binding to ' cboHem.DataBindings.Add("SelectedValue", myBindingSource, 'HemType") so I could use it; –  kirsten g Jan 13 at 23:15

Naming

HemEnum, or any enum name that ends with the word Enum, is a bad name for an enum. Enum types should not contain the word "enum" in their names.

Similarly, HemEnum values should not contain "Hem" in their names either. Your HemEnum should therefore read something like this:

public enum HemType
{
    None = -1,
    Sewn = 0,
    Weld = 1,
    DoubleFold = 2
}

myObject is a bad name for a class - should be MyObject. Kidding (although not really - types should be named following a PascalCasing convention). Anything that ends with "Object" should be banned from being a class name. Note that myObject doesn't compile as provided. (where's this.HemType?)

The pseudo-Hungarian notation is ok in for naming controls (i.e. the "cbo" prefix for ComboBoxes, "txt" for TextBoxes, etc.) - had you been using current technology (WPF) I would have strongly advised against such naming though.

Nitpicks

You seem to make static anything that can be made static. Don't. Just because a method doesn't use instance members now doesn't mean it never will, and changing a public static void method to be public void, is a breaking change.

vHemEnum doesn't seem to be a type defined anywhere. Typo?

I'll assume the out-of-whack indentation is a Copy+Paste glitch.

Binding?

You say you want databinding, and yet you're doing this:

foreach (var value in values)
{
    var s = GetHemTypeDescription((HemEnum)value.ID );
    cbo.Items.Add(s);
}

Create a type that exposes DisplayValue and EnumValue properties, and use @DanLyons' or @JesseCSlicer's answer to bind your combobox items.

Captions

A more extensible way (vs. attributes) of providing captions for your enums, would be to use a resource file (.resx) - call the strings per the enum's names, and then retrieve the caption from the resource strings:

var description = resx.ResourceManager.GetString(hemType.ToString());

Bottom line, I've seen worse WinForms code (and WPF for that matter), but it could be improved.

share|improve this answer

The easiest way to accomplish what you want is to create a list of the possible enum values and data bind that list to the combo box. You can do this through the designer (under Data) or with the following code:

cboHem.DataSource = enumList;

With an enum, it should automatically use the individual enum values as the selected value and display the enum values' .ToString result in the control.

If you use localization strings, it's a little (but not much) more complicated. Instead of binding enum values directly, you will want to bind a list of objects with the enum value and your localized string representation for the value, and then set the DisplayMember and ValueMember properties to the appropriate fields on your bound objects.

Once again, that can be done through the designer (under Data again) or through code, as follows:

cboHem.DisplayMember = "DisplayValue";
cboHem.ValueMember = "EnumValue";
share|improve this answer
    
How do I create enumList? I cant use Enum.GetValues(typeof(HemType)); because the description is different to HemType.ToString() –  kirsten g Jan 13 at 18:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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