Sponsored By Aspose - File Format APIs for .NET

Aspose are the market leader of .NET APIs for file business formats – natively work with DOCX, XLSX, PPT, PDF, MSG, MPP, images formats and many more!

Sometimes an enum is not the best idea

In our application we are dealing with a limited, but slowly growing number of suppliers. In some parts of the code each one is treated differently, in others parts we only have to identify them. Their id  which is also understood by the outer world, is a simple integer number. At first sight a nice and clean solution would be to use an enum.

public class Supplier

{

 

    public enum PublicIds

    {

        Company1 = 1,

        MyCompany = 5,

        ThatOtherCompany = 7

    }

 

All code can now use quite these descriptive identifiers.

public void DoWithCompany(PublicIds company)

{

    switch (company)

    {

        case PublicIds.Company1:

            // Do something with Company 1

            break;

        case PublicIds.MyCompany:

            // Do something with My Company

            break;

        case PublicIds.ThatOtherCompany:

            // Do something else

            break;

    }

}

<Update>

Note that this DoWithCompany method is not a member of the Company class. That would indeed be, as many a commenter has noted, be bad OO design. This method is a member of several quite different other classes and comes in many flavors. The suggestion to make the dowith behavior a polymorphic member of a Company would drag a lot of behavior into the company which does not belong there.

This company is a simple class with a simple ID member. The best type for this ID is an enum, this post is only about solving a problem with such an enum.

</Update>

 

But there are situations where an enum will hit back. In case a parameter of type object is expected the value will be that of the string representation of the enum’s member. For instance when you want to use the value as a parameter in a sql query.

cmdExists.Parameters.AddWithValue("@idSupplier", Supplier.PublicIds.MyCompany);

At first sight you are passing in the value 5 to the query. In reality you are passing in ‘MyCompany’, the ToString() representation of Supplier.PublicIds.MyCompany. This will result in a quite different query result as perhaps expected.

One workaround is to cast the enum member to the intended type.

cmdExists.Parameters.AddWithValue("@idSupplier", (int) Supplier.PublicIds.MyCompany);

Which has the risk of forgetting to do that.

Another solution is to drop the enum. Which can be done without losing the clarity of the code. It will still work using this class instead of the enum

public class PublicIds

{

    public const int Company1 = 1;

    public const int MyCompany = 5;

    public const int ThatOtherCompany = 7;

}

The only difference is the type of the parameter of the method, which is now an int instead on the enum type

public void DoWithCompany(int company)

{

    switch (company)

    {

        case PublicIds.Company1:

            // Do something with Company 1

            break;

        case PublicIds.MyCompany:

            // Do something with My Company

            break;

        case PublicIds.ThatOtherCompany:

            // Do something else

            break;

    }

}

You lose the type checking. When the company still was of type PublicId any call not passing in a member of that enum would throw an exception. Now any int value is accepted.

But you gain the certainty that the Id passed always has the intended value. Without sacrificing the readability of the code.

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • PeterGekko

    You are doing something different. In the case of the story the enum is used in the where part of the query. Select * from supplier where companyid = @value

    Which could result in the query
    Select * from supplier where companyid = ‘CompanyOne’
    or in the query
    Select * from supplier where companyid = 1.

    The first one being the unintended result of using an enum.

  • Heinzi

    Surprisingly, I cannot reproduce your result:

    enum EnumTest { EnumValue = 3 }

    static void Main(string[] args)
    {
    using (var cn = new SqlConnection(@”SERVER=myServer;Database=myDatabase;Integrated Security=SSPI”))
    {
    cn.Open();
    using (var cmd = cn.CreateCommand())
    {
    cmd.CommandText = “SELECT @value”;
    cmd.Parameters.AddWithValue(“@value”, EnumTest.EnumValue);
    Console.WriteLine(cmd.ExecuteScalar());
    }
    }
    Console.ReadLine();
    }
    This outputs “3”, not “EnumValue”, as I would have expected. I did a quick test using the parameter in the Where clause, and it also appeared to be used as an integer, not as a string.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    Thoughts ? Partly you describe parts of our situation, partly you don’t. The grouping is something which (in some situations) does.
    As stated elsewhere (ask first) it takes joining in on a team meeting.

    But also there: don’t start talking about database table yet.

  • Timothy Walters

    I’m curious as to what you’re really trying to achieve here, currently you have database ID’s for companies, of which some are given special meaning in some scenarios, but it’s not really the IDs that are special, but instead the Companies.

    Without knowing enough about your requirements I’m left wondering if you only include one company in a “group” for each special behaviour, or if multiple companies can be in the same “group”, either way it sounds like it might be an idea to create a “tags” or “groups” table and either have them include a reference to the company, or have the companies have a nullable GroupId, or have a many-to-many join.

    Your logic can then determine behaviour based on this group link (depending on which of the 3 you go for), and CompanyId can stay pure to it’s intent, identifying a company, not it’s behaviour.

    Thoughts?

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    Why ?

    The app exposes some of it’s functionality to the outside world. Members of the outside world have to be able to identify a “Supplier” without any ambiguity. That’s where we decided to introduce these id’s.

  • Joshka

    I don’t understand why you’re hard coding the supplier identifiers in your app. Perhaps a little bit more context about the actions being performed and how they differ per company would aid my understanding.

  • Brian

    I’m not advocating for the design, there’s something wrong somewhere else, maybe it’s just something that needs to be dealt with because a prior developer painted you in a corner.

    However, you could get the ease of enums without mucking around with all of this by declaring a company class that returned what you needed.

    public class Company
    {
    public Company(int id)
    {
    Id = id;
    }

    public int Id {get; private set;}

    public static Company Company1
    {
    get
    {
    return new Company(1);
    }
    }

    public static Company MyCompany
    {
    get
    {
    return new Company(5);
    }
    }

    public static Company ThatOtherCompany
    {
    get
    {
    return new Company(7);
    }
    }
    }

    So when you passed it into your command objects, you could say Company.MyCompany.Id. As for the switch, you could get rid of it by assigning a delegate within the company class getters (or in supplier) that acts on the object when you need it to, and specific to each company’s needs. Partial classes could also be a better design solution.

    Personally, I really hate switch statements unless I’m working on an actual enum, i.e. it’s something an object has a finite number of states for, and I’m doing something significantly different. For instance OrderStatus. In many cases generics will help me out of a jam like this. Ids in enums is a really bad example, and should be the first sniff to everyone that a better design is possible.

  • Ben

    I realize the update was put there to address concerns, my beef is with the self closing tag used to “Open” the update… I know, I get too caught up in the minutiae, but in a technical blog, the devil is in the details.

    Thanks for the post!

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    That update stopped most of the shells coming in. After that Steve was the first reco plane.

  • Ben

    I’m not surprised to see the amount of comments on this article. I figured it’d be cannon fodder after I saw this.


  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    At last, somebody asking what we are doing with the company or what the company is doing :)
    Still making far to many assumptions :(

    Few of our docwithcompanies are plain switches with a different case for every different company. Many treat several companies in the same manner, each dowith with their own grouping.
    Pulling this behavior into polymorphic behavior of the company itself would give the company reponsabilities which don’t belong there in the first place.

    All of this behavior is in other entities. The company is just a very simple bearer of some properties which _every_ company shares. Like an ID also known tot the outer world (like accounting). And that ID is what this post was about. Nothing else.

    Thanks for asking. And I would love to have an exhausting talk on our design. But with all the information at hand so we are all talking about the same thing.

  • YA Steve

    That switch is evil. “Puristic Evil”

    in DoWithCompany are you executing some polymorphic method under each case or are the methods unrelated. If they are unrelated, I would question the design of DoWithCompany. Are you for example calling a method such like SubmitOrder which then calls DoWithCompany for all the participating companies? If you then an event model would be better and each Company type would listen for the event and do the right thing in whatever method was responsible.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    That’s another interesting pattern. Response coing up in a new post

  • Andy Z.

    Why not just use a class hierarchy (as some have suggested) and the visitor pattern for your DoWithCompany functions. If you really wanted to stick with enums you could even use a modified visitor pattern where you write a static Visit function which takes the enum as a parameter and performs a switch to dispatch the calls.

    public interface ICompanyVisitor
    {
    void VisitCompany1();
    void VisitMyCompany();
    void VisitThatOtherCompany();
    }

    public static class CompanyDispatcher
    {
    static void Visit(this ICompanyVisitor visitor, PublicIds company)
    {
    switch (company)
    {
    case PublicIds.Company1:

    visitor.VisitCompany1();
    break;

    case PublicIds.MyCompany:

    visitor.VisitMyCompany();
    break;

    // etc.
    }
    }

    I do agree that enums in C# are not as useful as they should be and using them does expose you to certain problems, but there are always ways of improving the situation.

  • http://weblogs.asp.net/dotjosh Josh Schwartzberg

    Please excuse my grammar!

  • http://weblogs.asp.net/dotjosh Josh Schwartzberg

    @googly yes you could, I was trying to keep a code to a minimum even if that didn’t add much more code. I also generally stay from using implicit operators because it tends to confused the developers that end up inheriting your code.

  • Ole K

    @pvanooijen: you said the following about Josh’s suggestion:
    “Nice. But, as you say it requires “Value” so yoy have to change the syntax of the code which was used to talt to an enum”

    If he also added an override of the ToString() method, it would solve the “problem” you try to create in your example (cmdExists.Parameters.AddWithValue(“@idSupplier”, Supplier.PublicIds.MyCompany);), and that without even having to call the Value property.

  • googly

    @Josh: couldn’t you just add a public implicit operator int to get rid of the .Value property?

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    @Neslikkim
    An enum is based on an int by default, your qualifications is redundant. And it doesn’t solve the “implicit conversion” problem :(

    @Josh
    Nice. But, as you say it requires “Value” so yoy have to change the syntax of the code which was used to talt to an enum

    @Peter
    That’s nice, a quite new way to look at the problem. But it will require some refactoring. As said, the nice thing about my original approach is that it has the same syntax as an enum.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    Sorry for using the term “Puristic overkill”
    I think I see where we are talking in different directions. I have added a comment in the post, that should make IMHO clear where we lose each other.

  • http://peterspattern.com Peter

    Your problem seems constructed. Why not loose both approaches and go with a simple and clean class hierarchy
    (instead of a “puristic overkill” one):

    Given:

    public class Company
    {
    public int Id {get;private set;}

    public Company(int id) { Id = id; }
    }

    and:

    public static class SpecialCompanies
    {
    public static readonly Company CompanyX = new Company(5);
    public static readonly Company CompanyY = new Company(10);
    }

  • http://www.czarnovel.com/ home jobs

    helpful post for programmers.

  • http://www.vaibought.com Vaibhav

    Hi.
    You can also add a another function in your enum which will return the integer representation of the string like intValue(). Apply the object oriented approach to the enum that will good and enums are perfect in choosing this approach

  • http://www.py-sty.blogspot.com Steve Py

    Enum strikes again!

    It seems to be a hot topic for finding yourself on the receiving end of considerable criticism. :)

    Still, “puristic overkill”?!

    Sorry, but anytime you defend code logic that’s going to rely on a conditional logic to determine behaviour you better expect a slap across the back of the head. Enum does have it’s uses, though your example certainly doesn’t qualify as one of them. Complaints about their casting behaviour aside: as St. Dogbert would declare “Out OUT!! You demons of Stupiditiy!”

  • Steve

    “puristic overkill”

    Why is programming with objects consider purist overkill.

    I also had the thought that what you are trying to do with enums should be coded properly with subclassing.

    There are no shortcuts to good code. Do it right and you’ll appreciate it later, take shortcuts and it will bite you later.

    If that is ‘purism’, then I’ll take the purist route anyday :)

  • I blame my readers/commenters for my own problems!

    “I’m afraid this discussieon is taking the wrong direction because of the example.”

    Then why haven’t you fixed the example? Don’t blame your readers/commenters for your post’s numerous problems.

  • http://weblogs.asp.net/dotjosh Josh Schwartzberg

    Also to note in the above example: Since the default Equals() implementation uses Reference equality and the exposed “options” are static single-instance references… Equals() and == comparisons work out-of-the-box.

  • http://weblogs.asp.net/dotjosh Josh Schwartzberg

    I’d go for this approach, it’s not much more verbose and it gives you the same control that you get with an enum. Only downside is the using .Value to get the int…

    public class PublicIds
    {
    public static readonly PublicIds Company1 = new PublicIds(1);
    public static readonly PublicIds MyCompany = new PublicIds(5);
    public static readonly PublicIds ThatOtherCompany = new PublicIds(7);

    private PublicIds(int value)
    {
    Value = value;
    }

    public int Value { get; private set; }
    }

  • Marcel Popescu

    Advocating a crappy design on a site called CodeBetter is weird… and so is calling a proper OO design “puristic overkill”.

  • http://www.clear-lines.com/blog Mathias

    @pvanooijen: I know it’s not the point of the post, but it seems to me the example you picked is odd – and distracting! IMO, an enum is appropriate when you have a limited set of options, which are expected to cover all cases,. An enum where cases are tied to specific instances (Company1, MyCompany, ThatOtherCompany) seems really weird, because right off the bat you know there is no reason for this list to be exhaustive.

  • neslekkim

    Why not: ?
    public enum PublicIds : int
    {
    Company1 = 1,
    MyCompany = 5,
    ThatOtherCompany = 7
    }

  • Ole K

    Another good example why I prefer enums in Java over enums in C#…

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    @SinsI. Yep, that’s the core of my problem. And a fix for ADO.NET. I want to be sure, a fix which works for every API :) That’s why I chose to do this.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    I’m afraid this discussieon is taking the wrong direction because of the example.

    The only thing I wanted to show here is some unexpected behavior of an enum and a way to get around that.

    Some points:
    – This supplierID is an ID not the internal ID, it’s just a member
    – The DoWithSomething method is not a member of the supplier. There are a lot of DoWithSomethings. All over the app, not all of them switch different for every supplier. Not all of them fire of the same kind of things for every supplier.
    – And so on, and so on. It is a hornets nest. Again it’s just a member.

  • SinsI

    Oh, the joys of implicit type conversion!
    The problem is in AddWithValue() function, not enum.
    Thankfully, C# allows you to write a static function that recieves your enum:

    static void AddWithValue(this __SQLClassname, string id, PublicIds value)
    {
    return this->AddWithValue(id, (int)value);
    }

  • Ben Keeping

    It seems to me that you are breaking the Open-Closed Principle. Every time you need to add another supplier type or company, you will need to alter the “DoWithSomething” method.
    As Ole suggested, using sound OO design will mean your code is open for extension, but closed for modification. Its not “puristic overkill” – its sound design. If Supplier requires some specialisation, then extending Supplier to ThisSupplier, ThatSupplier etc will allow you to specialise your behaviour without having to alter every bit of code that has switch statements on the enum.

  • http://codebetter.com/members/pvanooijen/default.aspx pvanooijen

    @Ole,
    That could be a puristic overkill. The only thing we need is an ID, on everything else Suppliers might differ. Some kind of actions apply on two of them, some on three of them some on another two of them. It would be the first step to multiple inheritance or an umaintanable set of interfaces.

    @Ken
    That’s not true. An Enum is an int. The members only differs on their ToString implementation.
    ADO.NET does not take this in account. And I don’t know about any other API I’ll be sending my id’s to.

    @Ed. I would have to subclass supplierId from int. Which is sealed

  • Ed Blackburn

    I’m inclined to agree with Ole-Marius Moe-Helgesen’s suggestion; to apply Object-Orientation.

    If ADO.NET is a hang up use an implict operator overload in your base class to automatically box your CompanyId class into an integer.

  • http://www.kenegozi.com/blog Ken Egozi

    since enum is a string, you does not have real type safety check, so when you move to int, you did not lose anything.
    you could always pass ((PublicIds)n) where n is an int that is *not* a “valid” value within PublicIds enum, and the code will work.

    Actually, the only problem you “solve” by going from enum to a class with consts, is that of the wrongful use of ADO.NET
    You should always state types, and pass in the correct type. When using any proper ADO.NET wrapper it is being done for you. If you choose to use ADO.NET yourself, then use it properly

  • Ole-Marius Moe-Helgesen

    Why not go for an object oriented approach instead? Introduce subclasses of Supplied for each of your suppliers. With an Id property (which is of type int) that is overridden with a static return value. That would remove the use for those switch statements around your code while still giving you the benefits you are looking for (not having to cast to an int). It also maintains type safe interfaces since you won’t be allowed to create invalid supplier IDs.

    Introduce a facade to your external code that will map the external enumerator to the correct type and you only have to worry about that mapping in a single place.