patterncsharpMinor
C# product inventory. How would you refactor this?
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");
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
Make enumeration for menu choices
We'll use that in a minute.
Rename
Refactor
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
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 GetUserInputRefactor
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.