From 8a42d79bfdd06489a90504561fa6f63f51d88e2b Mon Sep 17 00:00:00 2001 From: Tom Date: Sun, 11 Jul 2021 15:28:12 +1200 Subject: [PATCH 1/2] notes --- Order.Management/Order.cs | 2 ++ Order.Management/PaintingReport.cs | 1 + Order.Management/Program.cs | 9 ++++++++- Order.Management/Shape.cs | 10 ++++++---- 4 files changed, 17 insertions(+), 5 deletions(-) diff --git a/Order.Management/Order.cs b/Order.Management/Order.cs index 235c789..a0d0db5 100644 --- a/Order.Management/Order.cs +++ b/Order.Management/Order.cs @@ -4,6 +4,8 @@ namespace Order.Management { + + /* I think Report HAS A Order rather than Report IS A Order would be better */ abstract class Order { public string CustomerName { get; set; } diff --git a/Order.Management/PaintingReport.cs b/Order.Management/PaintingReport.cs index 9b61c83..4459dbe 100644 --- a/Order.Management/PaintingReport.cs +++ b/Order.Management/PaintingReport.cs @@ -21,6 +21,7 @@ public override void GenerateReport() generateTable(); } + //too public, exposing implementation details public void generateTable() { PrintLine(); diff --git a/Order.Management/Program.cs b/Order.Management/Program.cs index 1422f85..325a1f3 100644 --- a/Order.Management/Program.cs +++ b/Order.Management/Program.cs @@ -3,15 +3,19 @@ namespace Order.Management { + // this class is too big and uncohesive should be decomposed class Program { // Main entry static void Main(string[] args) { + /* static methods are procedural, would be better to be decomposed into objects instead so can mock easier */ var (customerName, address, dueDate) = CustomerInfoInput(); var orderedShapes = CustomerOrderInput(); + + /* reports could be abstracted to share common interface */ InvoiceReport(customerName, address, dueDate, orderedShapes); CuttingListReport(customerName, address, dueDate, orderedShapes); @@ -19,9 +23,12 @@ static void Main(string[] args) PaintingReport(customerName, address, dueDate, orderedShapes); } + + /* these comments are just noise and arent actually helpful, should be deleted */ // Order Circle Input - public static Circle OrderCirclesInput() + public static Circle OrderCirclesInput()//too public { + /* probably shouled be DRYed up into an abstraction */ Console.Write("\nPlease input the number of Red Circle: "); int redCircle = Convert.ToInt32(userInput()); Console.Write("Please input the number of Blue Circle: "); diff --git a/Order.Management/Shape.cs b/Order.Management/Shape.cs index 7f5c61c..1fc3e20 100644 --- a/Order.Management/Shape.cs +++ b/Order.Management/Shape.cs @@ -4,20 +4,22 @@ namespace Order.Management { + //interface would be better (composition over inheritance) abstract class Shape { + //instance vars are too public and have no immutability public string Name { get; set; } public int Price { get; set; } - public int AdditionalCharge { get; set; } - public int NumberOfRedShape { get; set; } + public int AdditionalCharge { get; set; } //should probably hide implementation detail + public int NumberOfRedShape { get; set; }// I would suggest using decorater pattern something like IShape shape = new RedShape(new SquareShape(new Shape(3))); public int NumberOfBlueShape { get; set; } public int NumberOfYellowShape { get; set; } - public int TotalQuantityOfShape() + public int TotalQuantityOfShape() // would consider making shape smart enough to print itself in report rather than client of shape asking for quantity and then have client responsibile for printing it { return NumberOfRedShape + NumberOfBlueShape + NumberOfYellowShape; } - public int AdditionalChargeTotal() + public int AdditionalChargeTotal()// additional charge should probably be encapsulated away and only visible behaviour is price value { return NumberOfRedShape * AdditionalCharge; } From e93b6f8101ae3e35c828e48156abfecc3f8f2ad4 Mon Sep 17 00:00:00 2001 From: Tom Date: Mon, 12 Jul 2021 17:53:44 +1200 Subject: [PATCH 2/2] more notes --- Order.Management/PaintingReport.cs | 2 ++ Order.Management/Program.cs | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Order.Management/PaintingReport.cs b/Order.Management/PaintingReport.cs index 4459dbe..98c0255 100644 --- a/Order.Management/PaintingReport.cs +++ b/Order.Management/PaintingReport.cs @@ -22,6 +22,8 @@ public override void GenerateReport() } //too public, exposing implementation details + //this class knows too much about shapes, if just knew about abstract idea of shape and told it to print common behaviour would be easy to add another shape implementation in future + //at the moment all clients of shapes would need to be updated (Painting Report Invoice Report Cutting List Report) This is breaking Open Closed Principle public void generateTable() { PrintLine(); diff --git a/Order.Management/Program.cs b/Order.Management/Program.cs index 325a1f3..80cc3ad 100644 --- a/Order.Management/Program.cs +++ b/Order.Management/Program.cs @@ -28,7 +28,7 @@ static void Main(string[] args) // Order Circle Input public static Circle OrderCirclesInput()//too public { - /* probably shouled be DRYed up into an abstraction */ + /* probably shouled be DRYed up into an abstraction and use polymorphism */ Console.Write("\nPlease input the number of Red Circle: "); int redCircle = Convert.ToInt32(userInput()); Console.Write("Please input the number of Blue Circle: "); @@ -117,6 +117,7 @@ private static (string customerName, string address, string dueDate) CustomerInf // Get order input private static List CustomerOrderInput() { + //would loosely couple if was talking to an interface like IShape instead of these concretions Square square = OrderSquaresInput(); Triangle triangle = OrderTrianglesInput(); Circle circle = OrderCirclesInput();