Is having a Dictionary with one entry per member of an Enum a bad design choice?

Question

I have an Enum

enum Product{
Knife,
Fork,
Spoon
}

and a Dictionary with one entry per Enum member:

var productPrices = new Dictionary<Product, double>(){
{Product.Knife, 10},
{Product.Fork, 10},
{Product.Spoon, 15}
}

The Dictionary is supposed to contain the price for every product. At one point I loop through all products and get their prices

foreach(Product p in Enum.GetValues(typeof(Product))){
    var price = productPrices[p];
}

and this will of course throw an error if some product is not found in the Dictionary.

Is this a case of bad design, as the Dictionary is tightly coupled to the Enum, even though there is not formally any connection between the two? If, for example, someone was to add a new member to the Enum, they would need to be aware that they also would have to add a new entry to the Dictionary.

If it is a bad design choice, what is a better alternative?

Answer 1

When you use a strongly typed language like C#, it is better to create an object (struct, class, record) for this scenario, and using Dictionary is not a good option.

public enum ProductType{
  Knife,
  Fork,
  Spoon
}

public class Product
{
    public ProductType Type { get; set; }

    public decimal Price { get; set; }
}

public void PrintProducts()
{
    var products = new List<Product>()
     {
         new Product()
         {
             Type = ProductType.Fork,
             Price = 10
         },
         new Product()
         {
             Type = ProductType.Knife,
             Price = 10
         },
         new Product()
         {
             Type = ProductType.Spoon,
             Price = 10
         }
       };


     foreach (var product in products)           
         Console.WriteLine($"Product {product.Type} have a {product.Price} Price ");
        
 }

Answer 2

This design is good enough if you a) won't add new properties to products and b) keep the enum declaration and the dictionary together closely.

Answer 3

The design can be OK if you are ready to put up with restrictions:

1. Product can't have properties (e.g. Color, Manufactured, Material) since enum is nothing but an int value
2. You can't add a new Product (you should modify the code in two places)

If these options are too tight, why not implement a custom class?

using System.Collections.Concurrent;

...

// Simplest; you may want to add Equals, GetHashCode, ToString etc. 
public sealed class Product  {
  // Let have thread safe collection
  private static readonly ConcurrentDictionary<string, Product> s_AllProducts =
    new ConcurrentDictionary<string, Product>();

  // To be on the safer side, we expose readonly interface 
  public static IReadOnlyDictionary<string, Product> AllProducts => s_AllProducts;

  public string Name { get; }

  // Decimal is a better choice for money
  public decimal Price { get; }

  public Product(string name, decimal price) {
    Name = name ?? throw new ArgumentNullException(nameof(name));

    Price = price >= 0
      ? price
      : throw new ArgumentOutOfRangeException(nameof(price));

    // In case of Name conflict, let latter input win 
    s_AllProducts.AddOrUpdate(Name, this, (key, value) => this);
  }
}

Then you can use it as follow:

// We create new Products and they appear in the Product.AllProducts
new Product("Knife", 10);
new Product("Fork", 10);
new Product("Spoon", 10);

...

if (Product.AllProducts.Contains("Fork")) {
  ...
}

if (Product.AllProducts.TryGetValue("Knife", out var knife)) {
  Console.WriteLine($"{knife.Name} : {knife.Price}");
}

Answer 4

As with many things it depends.

If your list is small and rarely changed a dictionary can be a convenient choice. Another choice is a switch:

public static double GetPrice(this Product product) => 
    product switch{
       Product.Knife => 10,
       ...
       _=> throw ...
};

Both alternatives need to be updated if you add new enum values. The approach to handle this is to put the mapping between close to the enum-declaration (if possible), so anyone adding a new value has a easier time to spot that a new value should be added. You should also write unit tests to verify that each product has a price.

Another option is a class to "emulate" an enum:

public class Product{
    public double Price {get;}
    private Product(double price) => Price = price;
    public static Knife {get;} = new Product(10);
    ...
}

Since the constructor is private you can create a fixed set of values, but in most cases this should behave kind of like an extended enum.

For "real" systems where products need to be defined at runtime you probably need to use some type of database.