HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

C# product inventory. How would you refactor this?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisyouinventoryproductwouldhowrefactor

Problem

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 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");

Solution

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;
}

Code Snippets

public enum choices {Add = 1, Remove, Display, Exit}
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);
}
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;
}

Context

StackExchange Code Review Q#23428, answer score: 8

Revisions (0)

No revisions yet.