Herreray code review two #10
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Looking at section 5.3 of baeldung's clean code they show an example that shows a well-formed source file as an example. This is also explained further in rhamedy's summary of best practices where it is shown with in line code comments the order of what should be expected in a code source file for readability. For these reasons I have restructured the file to have the private variables declared in order of visibility, followed by the base constructor and overloaded in sequential order, and at the bottom with the methods, getters, and setters needed for the project. Sources: https://rhamedy.medium.com/a-short-summary-of-java-coding-best-practices-31283d0167d3 https://www.baeldung.com/java-clean-code
This method removed seemed to have an unused end goal. This was removed on the basis of unfocused code as described in the Characteristics of Clean Code described in baeldung. The result returns a null value and if it was used later in development it could serve potential unhandled nulls which would be harder to trace back to this unlogged null. With the code being unused and not serving current function it would be my recommendation to remove the method. Source: https://www.baeldung.com/java-clean-code
I have added changes after reviewing the examples of best practices for JavaDoc in code documentation tags for your methods. This could be used as a starting point towards higher code coverage using java doc to explain the parameters and the returns of your methods as this source suggests. Source: https://www.tutorialspoint.com/java/java_documentation.htm
discospiff
reviewed
Nov 8, 2021
Comment on lines
14
to
16
| public CartOrder(Food food, int foodQuantity) { | ||
| this.food = food; | ||
| } |
There was a problem hiding this comment.
Why is foodQuantity in the signature, but not used in the method?
discospiff
reviewed
Nov 8, 2021
Comment on lines
+18
to
+20
| public CartOrder(Food food){ | ||
| this.food = food; | ||
| this.foodQuantity = 1; |
There was a problem hiding this comment.
And here, foodQuantity not in signature, but is used in method?
|
Without cited sources, I'm not entirely clear if this branch reduces technical debt in a meaningful way. I'll leave the merge decision up to the team. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Analysis of the program.
I would say that the project has a pretty fleshed out application under development. I have seen good progress and merges since the last time I checked in code review one.
Was the program available in UC Github on time?
I was able to access and raise this pull request before the deadline of the assignment so that I could submit on time.
Is the program easy for you, as an outsider, to understand?
I suggested some changes that would lead to better outsider understanding - these also have an affect working towards lessening the technical debt in the project.
Does the program compile?
Yes the program has compiled some of the tests that I did some small work in for terminal based output testing.
This merge request contains the three commits made to the project.