initial commit for commenting on code review - could not create pull …#1
initial commit for commenting on code review - could not create pull …#1cxprofile wants to merge 2 commits intoSerkoOld:masterfrom
Conversation
…request so I forked on my repo
cxprofile
left a comment
cxprofile
left a comment
There was a problem hiding this comment.
It would also be nice to add .editorconfig file to specify custom editor settings for coding style consistency.
Also, the file naming convention of Order.Management project can be confusing when doing this:
Order.Management.Program
As we might mistake "Management" as a file/class name
Project File Name "Order.Management" can be renamed to OrderManagement
| public Circle(int red, int blue, int yellow) | ||
| { | ||
| Name = "Circle"; | ||
| base.Price = circlePrice; |
There was a problem hiding this comment.
- call to "base" could be removed as this call is redundant, as constructor of derived class implicitly calls the constructor for the base class - please implement in subsequent cases.
| { | ||
| Console.WriteLine("\nYour cutting list has been generated: "); | ||
| Console.WriteLine(base.ToString()); | ||
| generateTable(); |
There was a problem hiding this comment.
We need to exercise standard naming conventions using "PascalCase" for class/functions/method names and "camelCase" for instance variable, arrays, elements and parameter names. Please implement in subsequent cases
|
|
||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <TargetFramework>netcoreapp3.0</TargetFramework> |
There was a problem hiding this comment.
to eliminate warning from IDE using .NET version that has deprecated support, we can change to a higher supported version
Order.Management/Order.cs
Outdated
| { | ||
| return "\nName: " + CustomerName + " Address: " + Address + " Due Date: " + DueDate + " Order #: " + OrderNumber; | ||
| } | ||
| public override string ToString() => "\nName: " + CustomerName + " Address: " + Address + " Due Date: " + DueDate + " Order #: " + OrderNumber; |
There was a problem hiding this comment.
-
Using lambda expression/one-liners enhance performance and makes the code simpler - may be applied to this method because it does a simple return.
-
We can also implement additional interface class for Order to Inherit to simplify calls of report functions from the child classes CuttingListReport, InvoiceReport and PaintingReport.
Refer to attached sample implementations:
CuttingListReport.cs.txt
InvoiceReport.cs.txt
IReport.cs.txt
Order.cs.txt
PaintingReport.cs.txt
- Please also refer to additional fixes that are not included in this initial commit
( referring to above attached Order.cs.txt):
3.1 additional handling for null exception cases can be added to code
3.2 Fix on displaying OrderNumber is done - as this displays "0" when run.
3.3 applied usage of interpolated strings example:
row += $"{AlignCentre(column, width)}|";
| { | ||
| class Program | ||
| { | ||
|
|
There was a problem hiding this comment.
for readability purposes, we can add one new line per line of code to separate function/method/instance calls or simply group logical functions.
Order.Management/Program.cs
Outdated
| { | ||
|
|
||
| // Main entry | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "<Pending>")] |
There was a problem hiding this comment.
just adding this part to remove warnings on IDE - to implement "0 warning"
| 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.
-
Please apply proper naming Convention - change method name from "userInput()" to "UserInput()"
- please apply to all subsequent calls for this method
-
Exception Handling - This line would result to exception error when a character input is keyed in - would be good to add another function call that would handle the exception without using try{} catch{} because this is a simple method.
-
Attached a sample implementation of modified UserInput() for Program.cs for reference:
Program.cs.txt
| abstract class Shape | ||
| { | ||
| public string Name { get; set; } | ||
|
|
There was a problem hiding this comment.
standard coding practice - add new line for readability of function/method/instance declarations - please apply as needed
Order.Management/Shape.cs
Outdated
|
|
||
| public int NumberOfYellowShape { get; set; } | ||
|
|
||
| public int TotalQuantityOfShape() |
There was a problem hiding this comment.
- This method call can be converted to Property since method is simple
Kindly see attached sample implementation of Shapes.cs:
- Also, notice that we can remove unneeded "using" calls - please implement removal of unused "using" statement calls on all affected programs - these are usually greyed out by IDE.
|
|
||
| public int TotalAmountOfRedShapes() | ||
| { | ||
| return base.OrderedBlocks[0].NumberOfRedShape + base.OrderedBlocks[1].NumberOfRedShape + |
There was a problem hiding this comment.
as per comment regarding calls to "base"
1 participant
…request so I forked on my repo