Simple Product Inventory software using C# and OOP

Question

I did this small program, as part of a list of coding exercises. The list proposes the coding of a small software that's supposed to manage an inventory of products. Here's what is requested:

Product Inventory Project - Create an application which manages an inventory of products. Create a product class which has a price, id, and quantity on hand. Then create an inventory class which keeps track of various products and can sum up the inventory value.

This is the features I implemented in this application:

• Possibilty of adding products.
• Local storage of the data as JSON objects.
• List of all products.
• Possibilty of remove an specific product.
• You can clear the whoe inventory.
• Get the value of the inventory (sum of price of all items).
• Get the count of products in the inventory.
• Get the unit count (sum of quantity of all items).

I've created a Visual Studio solution with two projects:

• IvManager.ConsoleApp - The presentation part.
• IvManager.Business - Library containing the business logic.

Inside of IvManager.Business I have three classes:

Product - The product object that I have to manage.

Inventory - This is a static class that holds a list of products and has several methods:

• Load() - Private method that loads the data from the local file to the Inventory.Products list of products.
• Save() - Private method that saves the products list to the disk (in JSON format).
• RemoveProduct() - Remove an specific product accoding to its id.
• Add() - Add a new product to the inventory.
• GetNewId() - Gets an available id accoding to the items in the list.
• GetProductCount() - Get the count of products in the inventory.
• GetUnitCount() - Gets the sum of the quantity of all items.
• GetInventoryValue() - Sum of the price of all items.
• ClearInventory() - Removes all the items from the inventory.

DataManager - Private static class that handles save and recover data from the disk. It has two methods:

• LoadProducts() - Load data from the disk.
• SaveProducts() - Save data to the disk.

Code of Product.cs:

[Serializable]
public class Product
{                        
    public int Id { get; set; }
    public string Name { get; set; }
    public decimal Price { get; set; }
    public int Quantity { get; set; }
}

Code of Inventory.cs:

public static class Inventory
{
    public static List<Product> products;
    public static List<Product> Products
    {
        get
        {
            if (products.Count == 0)
            {
                Load();
            }

            return products;
        }
        set { products = value; }
    }

    static Inventory()
    {
        Products = new List<Product>();
    }

    private static void Load()
    {
        Products = DataManager.LoadProducts();
    }

    private static void Save()
    {
        DataManager.SaveProducts(Products);
    }

    public static void RemoveProduct(int productId)
    {
        Inventory.Products.RemoveAll(x => x.Id == productId);
        Save();
    }

    public static void Add(Product product)
    {
        Products.Add(product);
        Save();
    }

    public static int GetNewId()
    {
        int id;
        if (Inventory.Products.Count == 0)
            id = 1;
        else
        {
            id = Inventory.Products.Last().Id + 1;
        }

        return id;
    }

    public static int GetProductCount()
    {
        return Inventory.Products.Count();
    }

    public static int GetUnitCount()
    {
        return Inventory.Products.Select(x => x.Quantity).Sum();
    }

    public static decimal GetInventoryValue()
    {
        return Inventory.Products.Select(x => (x.Price * x.Quantity)).Sum();
    }
    public static void ClearInventory()
    {
        Inventory.Products.Clear();
        Save();
    }
}

Code of DataManager.cs:

static class DataManager
{
    private static string dataPath = "data.json";

    public static List<Product> LoadProducts()
    {
        List<Product> listOfProducts = new List<Product>();

        if (File.Exists(dataPath))
        {
            string json = File.ReadAllText("data.json");
            if (!string.IsNullOrWhiteSpace(json))
            {
                listOfProducts = JsonConvert.DeserializeObject<List<Product>>(json);
            }
        };           

        return listOfProducts;
    }        

    public static void SaveProducts(List<Product> productsToSave)
    {
        if (!File.Exists(dataPath))
            File.Create(dataPath);

        string json = JsonConvert.SerializeObject(productsToSave);

        File.WriteAllText(dataPath, json);
    }
}

Here's the full code, including the console application where the presentation is: https://gist.github.com/andradedearthur/20d6fc4b1325c11ecc7822e0bdb19fe8

I'm a student of C# and Object Orientation, so I would like to know what can be improved in this code. Probably the biggest challenge to me was to define in what class each method should go, and what should be the responsibilities of each class. Any feedback is welcome.

Answer 1

1. :

   private static void Save()
   {
       DataManager.SaveProducts(Products);
   }
   
   public static void RemoveProduct(int productId)
   {
       Inventory.Products.RemoveAll(x => x.Id == productId);
       Save();
   }
   
   public static void Add(Product product)
   {
       Products.Add(product);
       Save();
   }
   

   So every time you add a product or remove a product, ALL products are re-saved? What if you have millions of products? Isn't that overkill and inefficient? (Answer: Yes)

2. What's with everything being static? What's the good reason behind it? I can see a good reason for at least the Inventory class to not be static - it has state! Get rid of all that static stuff, then create an instance of each class you need and use it normally.

3. :

   public static int GetNewId()
   {
       int id;
       if (Inventory.Products.Count == 0)
           id = 1;
       else
       {
           id = Inventory.Products.Last().Id + 1;
       }
   
       return id;
   }
   

   This is a really bad idea because what's the guarantee that the products are always stored and loaded in the exact same order, AND that their order in the list is never changed? (None, especially when the list is publicly settable and modifiable.) You can easily end up with multiple products with the same ID.

4. DataManager should be a repository.

Answer 2

Few notes :

• Why do you have 2 public static List<Product> ? If one of them was intended to be a backing field it should be private instead of public, as like this you are not really encapsulating your property to any external accesses.

  private static List<Product> products;
  public static List<Product> Products
  {
      get
      {
          if (products.Count == 0)
          {
              Load();
          }
          return products;
      }
      set { products = value; }
  }
  

• Is there just 1 Inventory ? To me it sounds more like a normal class not a static one. You can still have public static members in non-static class.

• Redundant qualifier

  You don't need to explicitly point to the containing class all the time e.g Inventory.Foo() can become simply Foo() :

public static int GetNewId()
{
    int id;
    if (Products.Count == 0)
    {
        id = 1;
    }
    else
    {
        id = Products.Last().Id + 1;
    }

    return id;
}

You can shorten this even further :

public static int GetNewId()
{
    return Products.Count == 0 ? 1 : Products[products.Count - 1].Id + 1;
}

• Expression bodied members

  For the single lined return methods you can use expression bodies :

public static int GetNewId()
{
    return Products.Count == 0 ? 1 : Products[products.Count - 1].Id + 1;
}

public static int GetProductCount()
{
    return Products.Count;
}

public static int GetUnitCount()
{
    return Products.Select(x => x.Quantity).Sum();
}

public static decimal GetInventoryValue()
{
    return Products.Select(x => (x.Price * x.Quantity)).Sum();
}

Can become :

public static int GetNewId() => Products.Count == 0 ? 1 : Products[products.Count - 1].Id + 1;

public static int GetProductCount() => Products.Count;

public static int GetUnitCount() => Products.Select(x => x.Quantity).Sum();

public static decimal GetInventoryValue() => Products.Select(x => (x.Price * x.Quantity)).Sum();

But overall it looks pretty good, keep it up ! :)

Answer 3

I don't agree with your break down of the requirements

Product Inventory Project - Create an application which manages an inventory of products. Create a product class which has a price, id, and quantity on hand. Then create an inventory class which keeps track of various products and can sum up the inventory value.

I would take id as unique. You have a GetNewId() but use is not enforced and it is not even guaranteed to return a unique id.

public static List<Product> Products
{
    get
    {
        if (products.Count == 0)
        {
            Load();
        }

        return products;
    }
    set { products = value; }
}

Say the Load() actually returns 0. This will re-Load() with every get. It is static. Just load in the constructor.

Why even have a set? The would allow a use to create a new List<Product> and flat replace the existing list in whole.

public static void Add(Product product)

Does not check for unique id and even if it did it could be bypassed by just adding to directly to Products.

User could add or remove from Products directly and it would not be saved.

I think a better design is for Product to implement Object so you can enforce id as Equals. Then for the Products use a HashSet to enforce uniqueness.

The list of Product should be read only

There is not even a reason for Product to have public constructor

public int Id { get; set; }

User should not be able to modify Id

In summary the solution lacks inventory control