From 25550d8bc452b39a1cec88678a92d04635fe449d Mon Sep 17 00:00:00 2001 From: "SYSMEXNZ\\esmond" Date: Tue, 3 Aug 2021 12:11:58 +1200 Subject: [PATCH] Code review - Esmond --- Order.Management/CuttingListReport.cs | 1 + Order.Management/InvoiceReport.cs | 1 + Order.Management/Order.cs | 8 +++++++- Order.Management/PaintingReport.cs | 7 +++++++ Order.Management/Program.cs | 14 ++++++++++++++ Order.Management/Shape.cs | 5 +++++ Order.Management/Square.cs | 3 +++ 7 files changed, 38 insertions(+), 1 deletion(-) diff --git a/Order.Management/CuttingListReport.cs b/Order.Management/CuttingListReport.cs index 125d45f..1df9685 100644 --- a/Order.Management/CuttingListReport.cs +++ b/Order.Management/CuttingListReport.cs @@ -21,6 +21,7 @@ public override void GenerateReport() Console.WriteLine(base.ToString()); generateTable(); } + //Override new base abstract method public void generateTable() { PrintLine(); diff --git a/Order.Management/InvoiceReport.cs b/Order.Management/InvoiceReport.cs index 78443c3..4e8a2db 100644 --- a/Order.Management/InvoiceReport.cs +++ b/Order.Management/InvoiceReport.cs @@ -51,6 +51,7 @@ public void GenerateTable() PrintRow("Circle", base.OrderedBlocks[2].NumberOfRedShape.ToString(), base.OrderedBlocks[2].NumberOfBlueShape.ToString(), base.OrderedBlocks[2].NumberOfYellowShape.ToString()); PrintLine(); } + // private method with a better name (E.g. Print...) public void OrderSquareDetails() { Console.WriteLine("\nSquares " + base.OrderedBlocks[0].TotalQuantityOfShape() + " @ $" + base.OrderedBlocks[0].Price + " ppi = $" + base.OrderedBlocks[0].Total()); diff --git a/Order.Management/Order.cs b/Order.Management/Order.cs index 235c789..3e43e86 100644 --- a/Order.Management/Order.cs +++ b/Order.Management/Order.cs @@ -1,4 +1,5 @@ -using System; +// Remove unused references +using System; using System.Collections.Generic; using System.Text; @@ -6,16 +7,21 @@ namespace Order.Management { abstract class Order { + // Similar comment as Shape.cs public string CustomerName { get; set; } public string Address { get; set; } public string DueDate { get; set; } + // Not sure if this is even assigned? public int OrderNumber { get; set; } public List OrderedBlocks { get; set; } public abstract void GenerateReport(); + // Add a protected abstract method for GenerateTable with the implementation from PaintingReport/InvoiceReport + // Add new keyword to distinguish this method from the object.ToString() method public string ToString() { + // String interpolation and lamba return "\nName: " + CustomerName + " Address: " + Address + " Due Date: " + DueDate + " Order #: " + OrderNumber; } } diff --git a/Order.Management/PaintingReport.cs b/Order.Management/PaintingReport.cs index 9b61c83..d7becd5 100644 --- a/Order.Management/PaintingReport.cs +++ b/Order.Management/PaintingReport.cs @@ -6,6 +6,7 @@ namespace Order.Management { class PaintingReport : Order { + // private and prefix with underscore public int tableWidth = 73; public PaintingReport(string customerName, string customerAddress, string dueDate, List shapes) { @@ -21,6 +22,7 @@ public override void GenerateReport() generateTable(); } + // Remove method after refactor, see Order.cs public void generateTable() { PrintLine(); @@ -32,13 +34,16 @@ public void generateTable() PrintLine(); } + // Can be in a helper class public void PrintLine() { Console.WriteLine(new string('-', tableWidth)); } + // Can be in a helper class public void PrintRow(params string[] columns) { + // Validate columns parameter int width = (tableWidth - columns.Length) / columns.Length; string row = "|"; @@ -50,8 +55,10 @@ public void PrintRow(params string[] columns) Console.WriteLine(row); } + // Private method in helper class public string AlignCentre(string text, int width) { + // Validate parameters text = text.Length > width ? text.Substring(0, width - 3) + "..." : text; if (string.IsNullOrEmpty(text)) diff --git a/Order.Management/Program.cs b/Order.Management/Program.cs index 1422f85..9dec570 100644 --- a/Order.Management/Program.cs +++ b/Order.Management/Program.cs @@ -3,6 +3,9 @@ namespace Order.Management { + // Can use declare variables as var for variables where their type can be inferred from their assignment + // (but this is probably just based on preference) E.g. var circle = new Circle(redCircle, blueCircle, yellowCircle); + // Can remove comments from method names as it's adding additional lines which are redundant class Program { // Main entry @@ -19,16 +22,19 @@ static void Main(string[] args) PaintingReport(customerName, address, dueDate, orderedShapes); } + // Can change the access modifier to private as it's being used within this class (same as the other methods in this class) // Order Circle Input public static Circle OrderCirclesInput() { Console.Write("\nPlease input the number of Red Circle: "); + // More meaningful variable names (E.g. NumberOfRedCircles) int redCircle = Convert.ToInt32(userInput()); Console.Write("Please input the number of Blue Circle: "); int blueCircle = Convert.ToInt32(userInput()); Console.Write("Please input the number of Yellow Circle: "); int yellowCircle = Convert.ToInt32(userInput()); + // Combine return and assignment Circle circle = new Circle(redCircle, blueCircle, yellowCircle); return circle; } @@ -37,6 +43,7 @@ public static Circle OrderCirclesInput() public static Square OrderSquaresInput() { Console.Write("\nPlease input the number of Red Squares: "); + // Need exception handling or an int.TryParse to avoid crashing on invalid input (E.g. abc) int redSquare = Convert.ToInt32(userInput()); Console.Write("Please input the number of Blue Squares: "); int blueSquare = Convert.ToInt32(userInput()); @@ -61,19 +68,23 @@ public static Triangle OrderTrianglesInput() return triangle; } + // Method names should be Pascal case // User Console Input public static string userInput() { string input = Console.ReadLine(); + // Maybe use string.IsNullOrWhiteSpace() while (string.IsNullOrEmpty(input)) { Console.WriteLine("please enter valid details"); input = Console.ReadLine(); + // Remove extra line } return input; } + // Can create a factory class/method to create an ReportOrder for the 3 types of reports so we don't need to repeat ourselves // Generate Painting Report private static void PaintingReport(string customerName, string address, string dueDate, List orderedShapes) { @@ -107,6 +118,7 @@ private static (string customerName, string address, string dueDate) CustomerInf return (customerName, address, dueDate); } + // Needs a better method name (E.g. OrderShapes) // Get order input private static List CustomerOrderInput() { @@ -114,6 +126,8 @@ private static List CustomerOrderInput() Triangle triangle = OrderTrianglesInput(); Circle circle = OrderCirclesInput(); + // Can simplify into one statement by putting the shapes into the body of the shapes list + // E.g. new List { s1, s3, s3}; var orderedShapes = new List(); orderedShapes.Add(square); orderedShapes.Add(triangle); diff --git a/Order.Management/Shape.cs b/Order.Management/Shape.cs index 7f5c61c..37a16aa 100644 --- a/Order.Management/Shape.cs +++ b/Order.Management/Shape.cs @@ -4,8 +4,11 @@ namespace Order.Management { + // Does not need to be an abstract class anymore (see comment in Square.cs) abstract class Shape { + // Create a base class constructor + // Variables can have private setters and be set in the base class constructor public string Name { get; set; } public int Price { get; set; } public int AdditionalCharge { get; set; } @@ -17,10 +20,12 @@ public int TotalQuantityOfShape() return NumberOfRedShape + NumberOfBlueShape + NumberOfYellowShape; } + // Remove unused method public int AdditionalChargeTotal() { return NumberOfRedShape * AdditionalCharge; } + // See comment in Square.cs public abstract int Total(); } diff --git a/Order.Management/Square.cs b/Order.Management/Square.cs index 017601e..49712d0 100644 --- a/Order.Management/Square.cs +++ b/Order.Management/Square.cs @@ -9,6 +9,7 @@ class Square : Shape public int SquarePrice = 1; + // Can make use of the base constructor and pass in the common variables for assignment (See Shape.cs) public Square(int numberOfRedSquares, int numberOfBlueSquares, int numberOfYellowSquares) { Name = "Square"; @@ -19,6 +20,8 @@ public Square(int numberOfRedSquares, int numberOfBlueSquares, int numberOfYello base.NumberOfYellowShape = numberOfYellowSquares; } + // All the total methods seem to be the same for all the shapes, so they can be in the base class + // The sub total methods (E.g. RedSquaresTotal, BlueSquaresTotal etc) can be private methods public override int Total() { return RedSquaresTotal() + BlueSquaresTotal() + YellowSquaresTotal();