Code review - Esmond

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

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

Order.Management/Order.cs

@@ -1,21 +1,27 @@
-using System;
+// Remove unused references
+using System;
 using System.Collections.Generic;
 using System.Text;
 
 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<Shape> 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;
         }
     }

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<Shape> 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))

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<Shape> orderedShapes)
         {
@@ -107,13 +118,16 @@ 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<Shape> CustomerOrderInput()
         {
             Square square = OrderSquaresInput();
             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<Shape> { s1, s3, s3};
             var orderedShapes = new List<Shape>();
             orderedShapes.Add(square);
             orderedShapes.Add(triangle);

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

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