Gerardo Sullivan Code Review #12
Conversation
Add better accessibility, unused references, line spacing, Method naming conventions, etc.
Order should be its own concept as part of this application. Reports should be able to just take an order and output the details in a particular way
TODO: need to think of some auto generator for Order Number
Handle ints better, reuse code
| string address = GetUserInputAsString("Please input your Address: "); | ||
|
|
||
| // TODO: should probably be DateTime with valid user input for DateTime | ||
| string dueDate = GetUserInputAsString("Please input your Due Date: "); |
There was a problem hiding this comment.
Should be parsing user input into DateTime
| public string CustomerName { get; set; } | ||
| public string Address { get; set; } | ||
| public string DueDate { get; set; } | ||
| public string DueDate { get; set; } // TODO: should probably be DateTime |
| public string DueDate { get; set; } // TODO: should probably be DateTime | ||
| public int OrderNumber { get; set; } | ||
| public List<Shape> OrderedBlocks { get; set; } | ||
| public List<Shape> OrderedBlocks { get; set; } = new List<Shape>(); |
There was a problem hiding this comment.
OrderedBlocks data structure should be its own class with properties for each Shape type i.e.
public class OrderedBlocks
{
public List<Square> Squares { get; set; }
public List<Triangle> Triangles { get; set; }
public List<Circle> Circles { get; set; }
}
| public string ToString() | ||
| public override string ToString() | ||
| { | ||
| return "\nName: " + CustomerName + " Address: " + Address + " Due Date: " + DueDate + " Order #: " + OrderNumber; |
There was a problem hiding this comment.
nit: String interpolation better
| PrintRow("Square",base.OrderedBlocks[0].TotalQuantityOfShape().ToString()); | ||
| PrintRow("Triangle", base.OrderedBlocks[1].TotalQuantityOfShape().ToString()); | ||
| PrintRow("Circle", base.OrderedBlocks[2].TotalQuantityOfShape().ToString()); | ||
| PrintRow("Square", Order.OrderedBlocks[0].TotalQuantityOfShape().ToString()); |
There was a problem hiding this comment.
Order.OrderedBlocks[0] is very ugly and prone to bugs, find a more reliable data structure for each block type.
Quick fix is check list for type Square
e.g. Order.OrderedBlocks.Where(b => b is Square)
| class CuttingListReport : Order | ||
| public class CuttingListReport : Report | ||
| { | ||
| public int tableWidth = 20; |
There was a problem hiding this comment.
tableWidth can be const
| class InvoiceReport : Order | ||
| public class InvoiceReport : Report | ||
| { | ||
| public int tableWidth = 73; |
| { | ||
| public string Name { get; set; } | ||
| public int Price { get; set; } | ||
| public int AdditionalCharge { get; set; } |
There was a problem hiding this comment.
The current data structure only allows for one AdditionalCharge per shape.
Possible solution is for each attribute of the shape to have some AdditionalCharge (i.e. the colour and shape type)
| { | ||
| abstract class Shape | ||
| // TODO: rename this to Block | ||
| public abstract class Shape |
There was a problem hiding this comment.
May want colour to be its own class with properties (e.g. surcharge)
| public abstract void GenerateReport(); | ||
|
|
||
| public string ToString() | ||
| public override string ToString() |
There was a problem hiding this comment.
Prefer not to override the .net class ToString() method. Rather just have a Print() method or something like that
1 participant
No description provided.