wrong commit #94
wrong commit #94
Conversation
|
Hi @sallysohyun, your pull request title is not as specified. Please rectify the PR title to match the following requirement. The PR name should be in the format of
Please follow the above format strictly and edit your title for reprocessing. Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at nus-se-pr-bot and add a link to this PR. |
sallysohyun
changed the title
There was a problem hiding this comment.
@sallysohyun Good PR! I have added some suggestions and scope for improvements to reduce redundancy of several functions.
Overall, you did a good Job!
src/main/java/Duke.java
Outdated
| @@ -1,10 +1,21 @@ | |||
| import executor.task.TaskList; | |||
src/main/java/Duke.java
Outdated
| protected static TaskList taskList; | ||
|
|
||
| /** | ||
| * The Main method by which Duke will be launched. |
There was a problem hiding this comment.
Probably this can be deleted as it is not adding any value. Main methods are always unique and obvious to identify.
| @@ -0,0 +1,11 @@ | |||
| package duke.exception; | |||
|
|
|||
| public class DukeException extends Exception { | |||
There was a problem hiding this comment.
DukeException should the base exception class for other, more specific exception types related to the application. If you get DukeException error, it gives you no more information about the type other than the message string.
|
|
||
| public abstract class Command { | ||
| protected Boolean exitRequest = false; | ||
| protected CommandType commandType; |
There was a problem hiding this comment.
Maybe initialize the command type as a no-op type.
| public CommandAddIncomeReceipt(String userInput) { | ||
| this.commandType = CommandType.IN; | ||
| this.userInput = userInput; | ||
| this.description = "You can add a new income receipt in format of 'In $5.00 /tags tag'."; |
There was a problem hiding this comment.
This line seems too long. Maybe create a private static final string that stores this default description and assign it inside the constructor.
| generatedStr += " ("; | ||
| if (this.detailDesc != null && !this.detailDesc.isEmpty()) { | ||
| generatedStr += this.detailDesc + ": "; | ||
| } | ||
| if (this.taskDetails != null && !this.taskDetails.isEmpty()) { | ||
| generatedStr += this.taskDetails; | ||
| } | ||
| generatedStr += ")"; |
There was a problem hiding this comment.
Violation of Single Layer of Abstraction Principle (SLAP).
| int indexMsg = userInput.indexOf(' ', indexBackslash); | ||
| if (indexMsg >= 0) { | ||
| this.detailDesc = userInput.substring(indexBackslash + 1, indexMsg); | ||
| } | ||
| String[] splitDetails = userInput.split('/' + this.detailDesc, 2); | ||
| this.taskName = splitDetails[0].trim(); | ||
| if (splitDetails.length > 1) { | ||
| this.taskDetails = splitDetails[1].trim(); | ||
| } |
There was a problem hiding this comment.
Violation of Single Layer of Abstraction Principle (SLAP).
| * @return A String Array that contains all the types of this enum | ||
| */ | ||
| public static String[] getNames() { | ||
| TaskType[] holder = TaskType.values(); |
src/main/java/storage/Storage.java
Outdated
| while (scanner.hasNextLine()) { | ||
| try { | ||
| String loadedInput = scanner.nextLine(); | ||
| newTask = loadTaskFromStorageString(loadedInput); | ||
| taskList.addTask(newTask); | ||
| } catch (Exception e) { | ||
| System.out.println(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Violation of Single Layer of Abstraction Principle (SLAP).
src/main/java/storage/Storage.java
Outdated
| for (String taskString : taskStrings) { | ||
| if (newTask == null) { | ||
| newTask = TaskList.createTaskFromString(taskString); | ||
| } else { | ||
| queuedTask = TaskList.createTaskFromString(taskString); | ||
| queuedTasks.getList().add(queuedTask); | ||
| } |
There was a problem hiding this comment.
Violation of Single Layer of Abstraction Principle (SLAP).
…xceptions-currencyconverter Handled exceptions for Currency Converter to resolve issues AY1920S1-CS2113T-F09-1#199 , AY1920S1-CS2113T-F09-1#189 , AY1920S1-CS2113T-F09-1#183 , AY1920S1-CS2113T-F09-1#155 , AY1920S1-CS2113T-F09-1#130
…ser and prevent invalid input by user
…n-for-expenditure Added exceptions to expenditure
…k-change-help-formatting Added function to help command .
…ator Added simple arithmetic command line based functionalities
…r-exporter Tester exporter
New feature CommandEdit added with Junit testing
Signed-off-by: Mudaafi <[email protected]>
…CS2113T-F09-1#315) * Made some minor bugfixes. Signed-off-by: Mudaafi <[email protected]> * Moved Classes to their proper places. Signed-off-by: Mudaafi <[email protected]>
* Made some minor bugfixes. Signed-off-by: Mudaafi <[email protected]> * Fixed height issue. Signed-off-by: Mudaafi <[email protected]>
Code optimised and added some testing
…tor-again-export Done with refactor of export command
* Made some minor bugfixes. Signed-off-by: Mudaafi <[email protected]> * Fixed height issue. Signed-off-by: Mudaafi <[email protected]> * LAST BUGFIX Signed-off-by: Mudaafi <[email protected]>
…ll now be ignored. Signed-off-by: Mudaafi <[email protected]>
fixed comments