-
Notifications
You must be signed in to change notification settings - Fork 38
Clone solution for Code Review #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
There are some issues with the code of this PR.
|
| } | ||
|
|
||
| // Order Circle Input | ||
| public static Circle OrderCirclesInput() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OrderCirclesInput, OrderSquaresInput and OrderTrianglesInput are quite similar, we could abstract it to a generic method. example T OrderShapesInput<T>() where T: Shape.
Another suggestion is moving this logic to a separate factory class. It will help us to separate the logic out and make it testable.
| // Order Circle Input | ||
| public static Circle OrderCirclesInput() | ||
| { | ||
| Console.Write("\nPlease input the number of Red Circle: "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.Write is a static class, which means hard to test. We could create a ConsoleUserInterface class that implements the IUserInterface interface and wraps a call to the Console.Write method inside an instance method called Write.
| public static Circle OrderCirclesInput() | ||
| { | ||
| Console.Write("\nPlease input the number of Red Circle: "); | ||
| int redCircle = Convert.ToInt32(userInput()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intshould be replaced byvar. The same improvement could apply to similar areas in PR. For more information, please read here: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/implicitly-typed-local-variables- we should validate the user input before parsing it to an integer. I recommend extracting this logic and put it into a separate helper class (separate concern and unit testable)
| public static string userInput() | ||
| { | ||
| string input = Console.ReadLine(); | ||
| while (string.IsNullOrEmpty(input)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string which contains whitespace only should be invalid as well. Therefore, we should use string.IsNullOrWhiteSpace here.
|
|
||
| namespace Order.Management | ||
| { | ||
| abstract class Shape |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is missing public or internal
| public Square(int numberOfRedSquares, int numberOfBlueSquares, int numberOfYellowSquares) | ||
| { | ||
| Name = "Square"; | ||
| base.Price = SquarePrice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base is redundant in Square, Circle and Triangle classes
| return (base.NumberOfYellowShape * Price); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the difference between Square, Circle, and Triangle is Name, Price, and additional charge only. Therefore, we could use Shape abstract base class with properties NumberOfRedShape, NumberOfYellowShape, NumberOfBlueShape and function Total. The properties Name, Price, and AdditionalCharge could be defined as abstract. Also all shapes set the color shape numbers in the constructor. Therefore we could implement it in the base constructor
public class Shape {
public Shape(int numberOfRedSquares, int numberOfBlueSquares, int numberOfYellowSquares)
{
NumberOfRedShape = numberOfRedSquares;
NumberOfBlueShape = numberOfBlueSquares;
NumberOfYellowShape = numberOfYellowSquares;
}
public int NumberOfRedShape {get; }
...
public int Total() {
return RedSquaresTotal() + BlueSquaresTotal() + YellowSquaresTotal();
}
private int RedSquaresTotal()
{
return (NumberOfRedShape * Price);
}
private int BlueSquaresTotal()
{
return (NumberOfBlueShape * Price);
}
private int YellowSquaresTotal()
{
return (NumberOfYellowShape * Price);
}
public abstract string Name {get; }
...
}
| public int OrderNumber { get; set; } | ||
| public List<Shape> OrderedBlocks { get; set; } | ||
|
|
||
| public abstract void GenerateReport(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PaintingReport, CuttingListReport, or InvoiceReport are not different types of Orders. Instead, they are different types of Report services that could generate different reports from the Order. Therefore, we should move the GenerateReport method to an IReportService interface. PaintingReport, CuttingListReport, or InvoiceReport should be the derived class which implement the IReportService interface.
| generateTable(); | ||
| } | ||
|
|
||
| public void generateTable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generateTable method could be private. And for keeping the consistency, we should use pascal case naming, which is GenerateTable
| PrintLine(); | ||
| PrintRow(" ", " Red ", " Blue ", " Yellow "); | ||
| PrintLine(); | ||
| PrintRow("Square", base.OrderedBlocks[0].NumberOfRedShape.ToString(), base.OrderedBlocks[0].NumberOfBlueShape.ToString(), base.OrderedBlocks[0].NumberOfYellowShape.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All OrderedBlocks are the same structure, which is built by Square, Triangle, and Circle. So we could use a strong type for OrderedBlocks, such as
internal class OrderedBlocks
{
public Square Square { get; set; }
public Triangle Triangle { get; set; }
public Circle Circle { get; set; }
}
It will make the code more readable and not error-prone.
No description provided.