5
\$\begingroup\$

I am learning C# and I have made my way through the entire C# 101 course on buzz3D. The last project they have you do is create a product inventory system that must be split up between classes and only the Main() method can be in the Program class. It was listed that all methods had to be as close to or below 25 lines as possible. They also required you to use an example of exception handling and outputting/reading from a file.

With all that said, I was wondering how you might refactor this mess I seem to have made. It works good and does exactly as they wanted, but I can't help but think my 334 lines of code is a bit unnecessary for this application. I am trying to understand the proper use of enums, classes and methods, and any help would be great appreciated!

Please remember that I only finished a C# 101 class, so advanced refactoring will most likely confuse me greatly, but I am here to learn. They really only covered the basics of what I currently am using in this example. Basic looping with while, for, foreach, if...method declaration with basic parameter arguments, basic enum declaration and usage, and very basic class definitions including fields and properties. Thanks!

using System;
using System.IO;
using System.Collections.Generic;

namespace Product_Inventory_Using_Classes
{
    enum ProgramState { MainMenu, AddProduct, RemoveProduct, DisplayProducts, Exit }

    internal class Input
    {
        public ProgramState GetKey(string prompt)
        {
            while (true)
            {
                int choice;
                List<string> productCheck = WriteBuffer.ProductData;
                Console.Write(prompt);

                if (!int.TryParse(Console.ReadKey(true).KeyChar.ToString(), out choice))
                {
                    Console.WriteLine("\nInvalid choice");
                }
                if (choice != 1 && choice != 2 && choice != 3 && choice != 4)
                {
                    Console.WriteLine("\nInvalid choice");
                }
                else
                {
                    if (choice == 1)
                    {
                        return ProgramState.AddProduct;
                    }
                    if (choice == 2)
                    {
                        if (productCheck == null || productCheck.Count == 0)
                        {
                            Console.Write("No products exist. Add products first. Press any key");
                            Console.ReadKey(true);
                            return ProgramState.MainMenu;
                        }
                        return ProgramState.RemoveProduct;
                    }
                    if (choice == 3)
                    {
                        return ProgramState.DisplayProducts;
                    }
                    if (choice == 4)
                    {
                        return ProgramState.Exit;
                    }
                }
            }
        }

        public string GetString(string prompt)
        {
            Console.Write(prompt);
            return Console.ReadLine();
        }
    }

    internal class Menu
    {
        public ProgramState CurrentState { get; private set; }

        public Menu(ProgramState state)
        {
            CurrentState = state;
        }

        public void GetMenuState(ProgramState state)
        {
            CurrentState = state;
        }

        public ProgramState MainMenu()
        {
            Input choice = new Input();

            Console.Clear();
            Console.WriteLine("Main Menu\n\n" +
                              "[1] Add Product\n" +
                              "[2] Remove Product\n" +
                              "[3] Display Products\n" +
                              "[4] Exit\n");

            CurrentState = choice.GetKey("Enter Choice> ");

            return CurrentState;
        }

        public void AddProduct()
        {
            while (true)
            {
                WriteBuffer product = new WriteBuffer();

                Console.Clear();
                Console.WriteLine("Add Products\n");

                if (WriteBuffer.ProductData != null && WriteBuffer.ProductData.Count != 0)
                {
                    for (int i = 0; i < (WriteBuffer.ProductData.Count - 1); i += 2)
                    {
                        Console.WriteLine(WriteBuffer.ProductData[i]);
                    }
                }

                string itemToAdd = product.GetProductName();
                if (itemToAdd != "")
                {
                    product.GetProductPrice();
                }
                else
                {
                    break;
                }
            }
        }

        public void RemoveProduct()
        {
            List<string> product = WriteBuffer.ProductData;

            while (true)
            {
                Console.Clear();
                Console.WriteLine("Remove Products\n");

                for (int i = 0; i < (product.Count - 1); i += 2)
                {
                    Console.WriteLine(product[i]);
                }

                if (WriteBuffer.ProductData.Count == 0)
                {
                    break;
                }

                Input name = new Input();
                string itemRemoved = name.GetString("\nEnter item to be removed> ");

                if (itemRemoved != "")
                {
                    int itemLoc = WriteBuffer.ProductData.IndexOf(itemRemoved);
                    WriteBuffer.ProductData.RemoveAt(itemLoc + 1);
                    WriteBuffer.ProductData.RemoveAt(itemLoc);
                }
                else
                {
                    break;
                }
            }
        }

        public void DisplayProduct()
        {
            ConsoleOutput display = new ConsoleOutput();

            display.DisplayProducts();
        }
    }

    internal class WriteBuffer
    {
        public static List<string> ProductData { get; private set; } // the list is static so that it keeps the same data through all instances

        static WriteBuffer() // the constructor is static so the list is only instantiated once
        {
            ProductData = new List<string>();
        }

        public string GetProductName()
        {
            Input name = new Input();
            while (true)
            {
                string input = name.GetString("\nEnter Name> ");

                if (input != "")
                {
                    ProductData.Add(input);
                    return input;
                }
                return input;
            }
        }

        public void GetProductPrice()
        {
            Input price = new Input();

            while(true)
            {
                float validNum;
                if (float.TryParse(price.GetString("Enter Price> "), out validNum))
                {
                    ProductData.Add(validNum.ToString());
                    return;
                }
                Console.WriteLine("\nInvalid Data. Enter Price> ");
            }
        }
    }

    internal class FileIO
    {
        public static List<string> ProductList { get; private set; }
        public List<string> ProductListDisplay { get; private set; } 
        public string ProductName { get; private set; }
        public string ProductPrice { get; private set; }

        public void WriteFile()
        {
            ProductList = WriteBuffer.ProductData;

            using (StreamWriter productFile = new StreamWriter("Products.txt"))
            {
                WriteBuffer output = new WriteBuffer();

                foreach (var product in ProductList)
                {
                    productFile.WriteLine(product);
                }
            }
        }

        public void ReadFile()
        {
            try
            {
                using (StreamReader productFile = new StreamReader("Products.txt"))
                {
                    ProductListDisplay = new List<string>();
                    while (true)
                    {
                        ProductName = productFile.ReadLine();
                        ProductPrice = productFile.ReadLine();

                        ProductListDisplay.Add(ProductName);
                        ProductListDisplay.Add(ProductPrice);

                        if (ProductName == null)
                        {
                            break;
                        }
                    }
                }
            }
            catch (Exception)
            {
                StreamWriter productFile = new StreamWriter("Products.txt");
                productFile.Close();
            }
        }
    }

    internal class ConsoleOutput
    {
        public void DisplayProducts()
        {
            FileIO product = new FileIO();

            if (WriteBuffer.ProductData != null)
            {
                for (int i = 0; i < (WriteBuffer.ProductData.Count - 1); i += 2)
                {
                    product.ReadFile();
                    Console.WriteLine("{0} [${1}]", product.ProductListDisplay[i], product.ProductListDisplay[i + 1]);
                }
            }
        }
    }

    internal class Program
    {
        private static void Main()
        {
            Menu menu = new Menu(ProgramState.MainMenu);

            while (true)
            {
                switch (menu.CurrentState)
                {
                    case ProgramState.MainMenu:

                        menu.MainMenu();

                        break;

                    case ProgramState.AddProduct:

                        FileIO productAdd = new FileIO();

                        menu.AddProduct();
                        productAdd.WriteFile();

                        menu.GetMenuState(ProgramState.MainMenu);

                        break;

                    case ProgramState.RemoveProduct:

                        FileIO productRemoved = new FileIO();

                        menu.RemoveProduct();
                        productRemoved.WriteFile();

                        menu.GetMenuState(ProgramState.MainMenu);

                        break;

                    case ProgramState.DisplayProducts:

                        Console.Clear();
                        Console.WriteLine("Display Products\n");

                        menu.DisplayProduct();

                        Console.ReadKey(true);

                        menu.GetMenuState(ProgramState.MainMenu);

                        break;

                    case ProgramState.Exit:

                        return;
                }
            }

        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

That's a lot of code to look through :) I haven't analyzed all of it, but after looking at the first couple of classes, here are some ideas to get you started.

In your Input.GetKey method, you have a bunch of if statements to check the value of the choice variable. It would be much cleaner and easier to read if you used a switch statement, like this:

switch (choice)
{
    case 1:
        return ProgramState.AddProduct;
    case 2:
        if (productCheck == null || productCheck.Count == 0)
        {
            Console.Write("No products exist. Add products first. Press any key");
            Console.ReadKey(true);
            return ProgramState.MainMenu;
        }
        return ProgramState.RemoveProduct;
    case 3:
        return ProgramState.DisplayProducts;
    case 4:
        return ProgramState.Exit;
    default:
        Console.WriteLine("\nInvalid choice");
        break;
}

I do find it confusing, though, that the Input.GetKey method has logic in it that makes it very specific so it can only be used for the main menu. I would have expected all of the code for the main menu to be in one method. For instance:

public ProgramState MainMenu()
{
    Console.Clear();
    Console.WriteLine("Main Menu\n\n" +
                      "[1] Add Product\n" +
                      "[2] Remove Product\n" +
                      "[3] Display Products\n" +
                      "[4] Exit\n");
    while (true)
    {
        Console.Write("Enter Choice> ");
        int choice;
        if (!int.TryParse(Console.ReadKey(true).KeyChar.ToString(), out choice))
        {
            Console.WriteLine("\nInvalid choice");
        }
        else
        {
            switch (choice)
            {
                case 1:
                    return ProgramState.AddProduct;
                case 2:
                    if (productCheck == null || productCheck.Count == 0)
                    {
                        Console.Write("No products exist. Add products first. Press any key");
                        Console.ReadKey(true);
                        CurrentState = ProgramState.MainMenu;
                    }
                    else
                    {
                        ProgramState.RemoveProduct;
                    }
                    break;
                case 3:
                    CurrentState = ProgramState.DisplayProducts;
                    break;
                case 4:
                    CurrentState = ProgramState.Exit;
                    break;
                default:
                    Console.WriteLine("\nInvalid choice");
                    break;
            }
        }
    }
    return CurrentState;
}

And if you do remove that code from the Input class, it's kind of silly to even keep it around since it adds very little value over the Console class. The only valid reason I can think of to keep it would be if you want or need to abstract away the input method being used. For instance, if you want to be able to, down the road, change it from a console application to a web page or a WinForm application, then it would be easier to make that change if you had the Input class effectively wrapping the Console logic. However, if that's not important to you, as I suspect is the case, then you should probably just ditch the Input class entirely.

\$\endgroup\$
  • \$\begingroup\$ I totally agree with you. I wish I could ditch the whole input method, only issue is, that is what they requested for the project at the end of the series. Specifically each one of these fields had to be handled seperately: Storing Data, Reading Data, Writing Data, Getting input from user and displaying product data. \$\endgroup\$ – Zionmoose Mar 4 '13 at 13:50
  • \$\begingroup\$ If you do keep the Input class, then I would recommend keeping it very basic and reusable, and then make sure you are using it from everywhere. Nothing should use the Console class directly except for the Input class and the Output class. \$\endgroup\$ – Steven Doggart Mar 4 '13 at 13:59
  • 2
    \$\begingroup\$ For what it's worth, the typical way to layer your code would be to keep all input and output in "UI" or "Presentation" classes, all of the data-storage classes in "Data Model" or "DTO" classes, and all the reading and writing of the data in "Data Access" classes. \$\endgroup\$ – Steven Doggart Mar 4 '13 at 14:02
  • 1
    \$\begingroup\$ For instance, I would expect one or more data model classes that simply store all the data about a product in memory, nothing more. Then I would expect a ProductDataAccess class that loads and saves products to file. Then a ProductBusiness class that works with the data access and data model classes to perform all of the logic. Then a ProductUi class that contains methods to display products as well as input products and uses the ProductBusiness class to do all the work. Then maybe a MainMenuUi that displays the main menu and uses the ProductUi to do each task. \$\endgroup\$ – Steven Doggart Mar 4 '13 at 14:14
  • 1
    \$\begingroup\$ @StevenDoggart - When I implemented a menu like this, I declared a Dictionary<char, Tuple<string,Action>> to store the menu. The key was the character to select it, and the Tuple had the text to display and the function to call when that choice was selected. Entries looked like {'Q', Tuple.Create("Quit Program", QuitProgram)} with void QuitProgram() being one function. That's about the limit of complexity I'd want to have before making it into a class, though. \$\endgroup\$ – Bobson Mar 4 '13 at 19:20
8
\$\begingroup\$

Good Abstraction makes methods shorter

I suspect this is the hidden "Ah-Ha" lesson.

NOTE: the code samples are for inspirational purposes only. It is not a comprehensive refactoring of the entire program.

For example instead of if(choice !=1 && choice !=2 ...) do this if(ValidChoice()), that says what it does much better and ValidChoice() will be very short indeed.

Make enumeration for menu choices

public enum choices {Add = 1, Remove, Display, Exit}

We'll use that in a minute.

Rename GetString to something like GetUserInput

Refactor GetKey so code reads "abstractly appropriate"

public ProgramState GetKey(string prompt) {
    bool invalidChoice = true;

    while (invalidChoice){
        // all that "noisy" code goes into a method that says what it does
        choice = GetUserInput();
        invalidChoice = ValidChoice(choice);     
    }

    // no ELSE needed. We fall through when we get a valid choice
    return ProcessUserSelection(choice);
}


protected bool ValidChoice(Choices choice) {
    return (choice != Choices.Add && 
            choice != Choices.Remove && 
            choice != Choices.Display &&
            choice != Choices.Exit);
}

More refactoring. I made each case a method call; perhaps overkill (except for choice 2) but code in this method - "at this level of abstraction" - will not change if, for example, you change how AddProduct() works.

protected ProgramState ProcessUserSelection(Choices choice) {
    ProgramState stateOfTheUnion;  // default value is zero, by the way

    switch(choice) {
        case Choices.Add :
            stateOfTheUnion = AddProduct();
            break;

        case Choices.Remove:
            stateOfTheUnion = RemoveProduct();
            break;

        case Choices.Display:
            stateOfTheUnion = DisplayProduct();
            break;

        case Choices.Exit:
            stateOfTheUnion = Exit();
            break;

        default:
           choice = Choices.Exit;
           // whatever
    }

    return stateOfTheUnion;
}
\$\endgroup\$
  • \$\begingroup\$ This does not seem to work. Correct me if I am wrong here, but your ValidChoice(Choices choice) method declaration does not permit you to pass in an int or string for the comparison. This is due to ints and strings not being assignable to the Choices enum. So I am getting an error using your method because when I try to instantiate the ValidChoices(choice) method and assign it to invalidChoice, I get a compiler error that says you cannot assign an int to type Choices. \$\endgroup\$ – Zionmoose Mar 5 '13 at 7:54
  • \$\begingroup\$ Also even though you say the default value is 0, there is a compiler error that requires you to initialize it. \$\endgroup\$ – Zionmoose Mar 5 '13 at 9:58
  • \$\begingroup\$ From the code I illustrate, looks like GetUserInput() must convert the input to an enum. Converting is easy: Cast int to enum \$\endgroup\$ – radarbob Mar 5 '13 at 18:37
  • \$\begingroup\$ That's exactly what I had done. Thanks! Love google. \$\endgroup\$ – Zionmoose Mar 6 '13 at 2:01
1
\$\begingroup\$

From a high level, I have to disagree with your class design and names - they're really modeled after your implementation (Input, FileIO, etc.) instead of your problem domain. I would've expected to see some classes such as Product, Inventory, etc.

I won't attempt a straight refactoring - though there's isolated areas of code that could be improved on, the main thing that jumps out at me is the mix of concerns spread across all your classes. Currently, your classes all know way too much about Console and user input - you need to centralize that logic, and split into dedicated layers: UI, business, storage.

Here's an attempt to walk through a design - which should hopefully give you enough to tweak the specifics to meet your needs. Any compile errors or bugs are intentional and intended for learning purposes. Just kidding - though I will take some shortcuts since MarkDown isn't the best IDE.

Let's first extract out a Product class so we'll have something to work with:

 public class Product {
    public string Name { get; set; }
    public decimal Price { get; set; }
 }

Now, we'll need something to keep track of the inventory levels. We could use a simple List<Product> for this, but let's go ahead and abstract it out into an Inventory class, so that we'll have a place to put some convenience methods. Since we know we'll need to be able to remove an item by name (not caring about the price), we'll need some help to find that entry. We can also add some convenience methods around adding a product.

 public class Inventory {
     private List<Product> Products { get; set; }

     public int Count { get { return this.Products.Count; } }

     public IEnumerable<Product> GetAllProducts() {
         return this.Products.AsReadOnly(); // don't want products added/removed directly
     }

     public void RemoveByName(string productName) {
         var p = this.FindByName(productName);
         if (p != null) this.Products.Remove(p);
     }

     public void AddProduct(string productName, decimal productPrice) {
         this.Products.Add(new Product() { Name = productName, Price = productPrice });
     }

     public Product FindByName(string productName) {
        foreach (Product p in this.Products) {
            if (p.Name == productName) {
                return p;
            }
        }
        return null; // not found
     }
 }

Ok - that pretty much gives us our product management functionality. Now, on to UI.

You've got the right idea here - console input/output should be handled generically, so let's expand on that idea and write a few generic helper classes. A good way to think through this is figuring out what portions of what you need to do are specific to your app, and what is a generic concern. You should try to split those as much as possible. These won't have anything to do with Products, so they'll be reusable in a different console app.

 public class ConsoleInput {
      public string Ask(string prompt) {
          this.Tell(prompt);
          return Console.ReadLine();
      }

      public decimal AskDecimal(string prompt) {
           decimal d;
           string answer;
           do {
              answer = this.Ask(prompt);
           } while (!decimal.TryParse(answer, out d));
           return d;
      }

      public int AskInt(string prompt) {
           int i;
           string answer;
           do {
              answer = this.Ask(prompt);
           } while (!int.TryParse(answer, out i));
           return i;
      }

      public string Choose(string prompt, params string[] validChoices) {
          string answer;
          do {
              answer = this.Ask(prompt);
          } while (validChoices.IndexOf(answer) < 0);
          return answer;
      }

      public int Choose(string prompt, params int[] validChoices) {
          int answer;
          do {
              answer = this.AskInt(prompt);
          } while (validChoices.IndexOf(answer) < 0);
          return answer;
      }

      public string Tell(string message) {
          Console.Write(message);
      }
 }

Ok, now we have enough to get to refactoring the Menu class. Currently, your Menu class has too many responsibilities - it's showing the menu, tracking the current state, and adding/removing products. We're going to trim that down to just showing the menu and tracking the current state:

 public class Menu {
       public ProgramState CurrentState { get; private set; }

       public void MainMenu() {
        ConsoleInput input = new ConsoleInput();

        this.CurrentState = (ProgramState)input.Choose(
            "Main Menu\n\n" +
            "[1] Add Product\n" +
            "[2] Remove Product\n" +
            "[3] Display Products\n" +
            "[4] Exit\n",
            1, 2, 3, 4
        );
    }
 }

Ok - now we've got most of the pieces, we just need to put the actual program logic together. Let's move on to the Main method:

 public class Program {
     public static void Main(string[] args) {
          Inventory inventory = new Inventory();
          ConsoleInput input = new ConsoleInput();
          Menu menu = new Menu();

          switch (menu.CurrentState) {
             case ProgramState.MainMenu:
                 menu.MainMenu();
                 break;
             case ProgramState.AddProduct:
                 inventory.AddProduct(
                     input.Ask("Product Name?"),
                     input.AskDecimal("Product Price?")
                 );
                 menu.MainMenu();
                 break;
             case ProgramState.RemoveProduct:
                 if (inventory.Count == 0) {
                     input.Tell("No products exist. Add products first!");
                 } else {
                     inventory.RemoveProduct(
                        input.Ask("Product Name?")
                     );
                 }
                 menu.MainMenu();
                 break;
             case ProgramState.DisplayProducts:
                 foreach (Product p in inventory.GetAllProducts()) {
                     input.Tell("{0} {1:c2}\n", p.Name, p.Price);
                 }
                 menu.MainMenu();
                 break;
             case ProgramState.Exit:
                 return 0;
          }
     }
 }

For file IO, the way you're currently doing it (saving after each add/remove) you could add it as part of Inventory:

 public class Inventory {
     private string FileName { get; set; }

     public Inventory(string fileName) {
         this.FileName = fileName;
         this.Products = this.ReadProducts();
     }

     public void AddProduct(...) {
         ...
         this.SaveProducts();
     }

     private void SaveProducts()
         using (StreamWriter ...) {
         }
     }

     private List<Product> ReadProducts() {
         using (StreamReader ...) {
         }
     }

If you're familiar with inheritance, hopefully you can see how you could add that functionality into a 'PersistentInventory' subclass.

Hopefully, you can see by splitting the functionality out cohesively both readability and maintainability. Think through adding the following functionality in v2 of your app with the various designs:

  • Configurable filenames for storage
  • Multiple inventory locations (eg., store/warehouse)
  • Keeping track of quantity levels as products are added/removed
  • Sending an alert (eg., via email) when quantity reaches 0 for an item
  • Making a web page interface
\$\endgroup\$

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.