3
\$\begingroup\$

Our database has several tables that have a value that can be one of 4 things, so we made these columns instead have an id column with a foreign key to an enum table with an id and a value column to hold those 4 values. When interfacing this through an ORM, I came up with the following enum-like class to manage keeping things in sync:

using DataAccess.ECI.Models;
using System;
using System.Collections.Generic;
using System.Text;
using System.Linq;

namespace DataAccess.ECI
{

    public class StatusEnum
    {
        private string type;
        private static Dictionary<string, Status> lookupCache = null;
        private static List<StatusEnum> members= new List<StatusEnum>();
        private StatusEnum(string type)
        {
            this.type = type;
            members.Add(this);
        }
        //the text needs to be kept consitent with the Status table
        public static readonly StatusEnum ACTIVE = new StatusEnum("ACTIVE");
        public static readonly StatusEnum COMPLETED= new StatusEnum("COMPLETED");
        public static readonly StatusEnum DELETED= new StatusEnum("DELETED");
        public static readonly StatusEnum INPROGRESS= new StatusEnum("INPROGRESS");

        public static void Init(ECIContext ctx)
        {
            lookupCache=new Dictionary<string, Status>();
            foreach(StatusEnum member in members)
            {
                try
                {
                    lookupCache.Add(member.type, ctx.Status.Single(s => s.Name == member.type));
                }catch(Exception ex)
                {
                    throw new InvalidOperationException($"No record found with Name {member.type}",ex);
                }
            }
        }
        public Status asDBType()
        {
            if (lookupCache == null) throw new InvalidOperationException("Init(ECIContext) has not been called for this enum");
            return lookupCache[type];
        }
        public override string ToString()
        {
            return type;
        }
    }
}

I thought it works pretty well, but I'm concerned about a few things.

  1. since it's mostly static methods, it forces lots of duplication when other enum tables are added that need the same code wrapper
  2. if we forget to call init, this error isn't caught until late in the runtime

I would appreciate any comments on the code and suggestions for how to make it better.

EDIT:

for more context, we are using aspnet core, so the context is added in Startup.cs:ConfigureServices(IServiceCollection services) :

...
services.AddDBContext<ECIContext>(options=>
    options.UseSqlServer(/*connection string from config*/)
);
...

and Init is currently called in Startup.cs:Configure(IApplicationBuilder app,...):

...
ECIContext ctx=app.ApplicationServices.GetService<ECIContext>();
StatusEnum.Init(ctx);
...

this enum will be used like

ctx.Document.add(new Document{
    PageCount=4,
    Status=StatusEnum.ACTIVE.asDBType(),
    ...other properties...
});

which works because our ORM, entity framework core, adds a field for the foreign key linked table, and stores the id when appropriate

\$\endgroup\$
4
\$\begingroup\$

So your Init threw me off, initially I thought this class was doing something, but it was doing something else (not your fault, totally mine).

Here's how I would handle this:

  • Get rid of asDBType and just return the Status, we'll see how in a moment.
  • Don't call the private member type if the database member is Name, call it name.
  • Use CallerMemberNameAttribute so you don't need a magic string.

You want type-safety at compile time, against something that's inherently unsafe, so we have to cheat quite a bit to make that happen.

We're also going to use some Reflection - doing so means we can eliminate the List<StatusEnum> members, and replace a lot of this class with a couple quick calls.

public class StatusEnum
{
    private static Dictionary<string, Status> _lookup = new Dictionary<string, Status>();
    private static Status _get([CallerMemberName]string name = null) =>
        _lookup.Length > 0 ? _lookup[name] : throw new InvalidOperationException($"The method '{nameof(Init)}' has not been called.");
        // `CallerMemberNameAttribute` gives us the name of whatever
        // method or property called this method, which means since
        // our property calls `_get()`, we get the name of that
        // property implicitly

    public static Status ACTIVE => _get();
    public static Status COMPLETED => _get();
    public static Status DELETED => _get();
    public static Status INPROGRESS => _get();

    public static void Init(ECIContext ctx)
    {
        foreach (var prop in typeof(StatusEnum).GetProperties())
        {
            var name = prop.Name;

            // Assuming `ctx.Status` has a `SingleOrDefault`
            var status = ctx.Status.SingleOrDefault(x => x.Name == name);
            if (status == null) { throw new ArgumentException($"The value '{name}' is not a valid '{nameof(Status)}'."); }

            _lookup.Add(name, status);
        }
    }
}

Realistically, your StatusEnum class is a bunch of helper stuff, so you don't really need instance members, and you don't really want to keep an instance of this class, you care about the Status class. This is similar to how I do Setting objects in Entity Framework, though my setup was more complex.

Your version requires adding a property with a self-referencing name, this version eliminates that.

public static Status ONHOLD => _get();
public static readonly StatusEnum ONHOLD = new StatusEnum("ONHOLD");

Also, in your version, C#6.0 introduced nameof() which would allow you to safely initialize:

public static readonly StatusEnum ACTIVE = new StatusEnum(nameof(Active));

The other advantage to this version is you'll always be sure that you have a valid Status object, no need to asDBType, which means we can make this generic for your other situations:

public abstract class ItemizedEnum<TItem, TClass>
    where TClass : ItemizedEnum<TItem, TClass>
{
    private static Dictionary<string, TItem> _lookup = new Dictionary<string, TItem>();
    protected static TItem Get([CallerMemberName]string name = null) =>
        _lookup.Count > 0 ? _lookup[name] : throw new InvalidOperationException($"The '{nameof(Init)}' method MUST be called first.");

    protected abstract TItem GetFromDatabase(ECIContext ctx, string name);

    public static void Init(ECIContext ctx)
    {
        var classInstance = Activator.CreateInstance<TClass>();
        foreach (var prop in typeof(TClass).GetProperties())
        {
            var name = prop.Name;
            var obj = classInstance.GetFromDatabase(ctx, name);
            if (obj == null || obj.Equals(default(TItem)) == true) { throw new ArgumentException($"The value '{name}' is invalid for '{typeof(TClass).Name}'"); }
            _lookup.Add(name, obj);
        }
    }
}

Finally, in StatusEnum:

public class StatusEnum : ItemizedEnum<Status, StatusEnum>
{
    public static Status ACTIVE => Get();
    public static Status COMPLETED => Get();
    public static Status DELETED => Get();
    public static Status INPROGRESS => Get();

    protected override Status GetFromDatabase(ECIContext ctx, string name) =>
        ctx.Status.FirstOrDefault(x => x.Name == name);
}

This decreases the level of redundancy, the class only needs to implement a GetFromDatabase(ECIContext, string) method, and we now no longer care about any of that in StatusEnum itself, just what values it contains. We can then build other ItemizedEnum classes:

public class StateEnum : ItemizedEnum<State, StateEnum>
{
    protected override State GetFromDatabase(ECIContext ctx, string name) =>
        ctx.State.FirstOrDefault(x => x.Title == name);

    public static State OtherTest => Get();
}

And we get the same type-safety. Now a test of your live examples might be:

public class StatusEnum : ItemizedEnum<Status, StatusEnum>
{
    protected override Status GetFromDatabase(ECIContext ctx, string name) =>
        ctx.Status.FirstOrDefault(x => x.Name == name);

    public static Status Test => Get();
}

public class StateEnum : ItemizedEnum<State, StateEnum>
{
    protected override State GetFromDatabase(ECIContext ctx, string name) =>
        ctx.State.FirstOrDefault(x => x.Title == name);

    public static State OtherTest => Get();
}

StatusEnum.Init(new ECIContext());
StateEnum.Init(new ECIContext());
Console.WriteLine(StatusEnum.Test.Name);
Console.WriteLine(StateEnum.OtherTest.Title);
\$\endgroup\$
  • \$\begingroup\$ this looks like it solves my first concern nicely, but I realized I only need this approach for one of the enums, where LogLevel needs an explicit ordering and I don't want to tie it to the id, the rest I should be able to just use the id directly \$\endgroup\$ – Austin_Anderson Sep 26 '17 at 15:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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