[AY1920S1-CS2113T-F11-2] Dolla#39
Open
omupenguin wants to merge 895 commits intonusCS2113-AY1920S1:masterfrom
Open
[AY1920S1-CS2113T-F11-2] Dolla#39omupenguin wants to merge 895 commits intonusCS2113-AY1920S1:masterfrom
omupenguin wants to merge 895 commits intonusCS2113-AY1920S1:masterfrom
Conversation
x3chillax
referenced
this pull request
in x3chillax/main
Oct 16, 2019
andyrobert3
reviewed
Oct 20, 2019
andyrobert3
left a comment
andyrobert3
left a comment
There was a problem hiding this comment.
Good progress so far, the team has written significant amount of features
These are the pointers that I saw:
- Enums can be used, especially for
Tasktype - Commented code should be removed to improve readability
- Catch blocks should handle exceptions in a meaningful manner - https://stackify.com/best-practices-exceptions-java/ for tips on how to handle exceptions.
| String modeToModify = mode.split(" ")[1]; | ||
| if (modeToModify.equals("entry")) { | ||
| if (verifyAddCommand() == true) { | ||
| return new ModifyEntryCommand(inputArray[1], stringToDouble(inputArray[2]), inputArray[3], date); |
There was a problem hiding this comment.
1, 2, 3 index are magic numbers that can be replaced with variables
Author
There was a problem hiding this comment.
Could you provide an example regarding this?
| * Checks if the first word after 'add' is either 'income' or 'expense'. | ||
| * @param s String to be analysed. | ||
| * @return Either 'expense' or 'income' if either are passed in. | ||
| * @throws Exception ??? |
There was a problem hiding this comment.
may want to specify exception type & details on it
Author
|
PR was not tracked last week even though it was created a while back. |
Added dolla exception to handle dolla related exceptions
Adding tests
Added support for fully modifying limits, fixed bugs, and slightly refactored codebase.
rewrite verify debt command
# Conflicts: # src/main/java/dolla/parser/DebtsParser.java # src/main/java/dolla/parser/Parser.java
Added DebtList Test and Rewrite verifyDebtCommand
Updated Ui image in readme
Add test for ActionUi and DebtUi
Add Logging Support
Fixed bugs with view.
added UiTest
add bill to help
nishanthelango
added a commit
to nishanthelango/Duchess
that referenced
this pull request
Nov 23, 2021
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
@yetong1895
@Weng-Kexin
@tatayu