[AY1920S1-CS2113T-W13-2] JavaCake #88
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
flxffy
reviewed
Oct 21, 2019
There was a problem hiding this comment.
Overall, pretty good job and your code is quite readable.
Things to note:
- Variable names can be more descriptive.
- Remnants of Duke, especially the exceptions.
- A lot of magic numbers.
- Some methods are really long.
- Code that has been commented out can be removed.
- Make use of super class constructors and use proper
@Overrideannotations.Questionand its sub-classes were really well done, you can take a look at that! - Not a lot of tests, but what you have now is a good start!
src/main/java/javacake/Parser.java
Outdated
Comment on lines
41
to
64
| if (input.equals("exit")) { | ||
| return new ExitCommand(); | ||
| } else if (input.equals("list")) { | ||
| return new ListCommand(); | ||
| } else if (input.equals("back")) { | ||
| return new BackCommand(); | ||
| } else if (input.equals("help")) { | ||
| return new HelpCommand(inputCommand); | ||
| } else if (input.equals("score")) { | ||
| return new ScoreCommand(); | ||
| } else if (input.equals("reset")) { | ||
| return new ResetCommand(); | ||
| } else if (input.equals("goto")) { | ||
| if (inputCommand.length() <= 4) { | ||
| throw new DukeException("Please specify index number in 'goto' command!"); | ||
| } | ||
| return new GoToCommand(inputCommand.substring(5)); | ||
| } else if (input.equals("tree")) { | ||
| return new MegaListCommand(); | ||
| } else if (input.equals("deadline")) { | ||
| return new AddCommand(inputCommand); | ||
| } else { | ||
| throw new DukeException("OOPS!!! I'm sorry, but I don't know what that means."); | ||
| } |
There was a problem hiding this comment.
There's a better way to do this! Good attempt though, it looks neat :)
| /** | ||
| * Stores all files in the currentFilePath into listOfFiles. | ||
| */ | ||
| public void loadFiles() throws DukeException { |
There was a problem hiding this comment.
This is a very long method. Look up ways to refactor code.
Comment on lines
28
to
30
| public ProgressStack() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Empty constructor? Look up default constructors.
Comment on lines
159
to
161
| String finalTrim = reducedFilePath.toString(); | ||
| finalTrim = finalTrim.substring(0, finalTrim.length() - 1); | ||
| return finalTrim; |
There was a problem hiding this comment.
finalTrim is a very vague variable name, it could be more descriptive.
Comment on lines
41
to
68
| /*switch (type) { | ||
| case TODO: | ||
| ui.showMessage(Parser.runTodo(progressStack.getData(), input, Parser.TaskState.NOT_DONE)); | ||
| storage.write(progressStack.getData()); | ||
| break; | ||
| case DEADLINE: | ||
| ui.showMessage(Parser.runDeadline(progressStack.getData(), input, Parser.TaskState.NOT_DONE)); | ||
| storage.write(progressStack.getData()); | ||
| break; | ||
| case EVENT: | ||
| ui.showMessage(Parser.runEvent(progressStack.getData(), input, Parser.TaskState.NOT_DONE)); | ||
| storage.write(progressStack.getData()); | ||
| break; | ||
| case DAILY: | ||
| ui.showMessage(Parser.runRecurring(progressStack.getData(), input, Parser.TaskState.NOT_DONE, "daily")); | ||
| storage.write(progressStack.getData()); | ||
| break; | ||
| case WEEKLY: | ||
| ui.showMessage(Parser.runRecurring(progressStack.getData(), input, Parser.TaskState.NOT_DONE, "weekly")); | ||
| storage.write(progressStack.getData()); | ||
| break; | ||
| case MONTHLY: | ||
| ui.showMessage(Parser.runRecurring(progressStack.getData(), input, Parser.TaskState.NOT_DONE, "monthly")); | ||
| storage.write(progressStack.getData()); | ||
| break; | ||
| default: | ||
| throw new DukeException(" [Unknown COMMAND TYPE]"); | ||
| }*/ |
There was a problem hiding this comment.
Don't leave commented out code. Just go ahead and delete unused code. Now that we are using a revision control system, we can recover any deleted code later.
Comment on lines
24
to
74
| /** | ||
| * Constructor for profile. | ||
| * @param filename String of filepath | ||
| * @throws DukeException when unable to create profile | ||
| */ | ||
| public Profile(String filename) throws DukeException { | ||
| try { | ||
| File file = new File("data/save/savefile.txt"); | ||
| Duke.logger.log(Level.INFO,"Filepath: " + filepath); | ||
| try { | ||
| if (!file.getParentFile().getParentFile().exists()) { | ||
| file.getParentFile().getParentFile().mkdir(); | ||
| file.getParentFile().mkdir(); | ||
| file.createNewFile(); | ||
| initialiseUser(); | ||
| System.out.println("A" + file.getParentFile().getParentFile().getPath()); | ||
| } else if (!file.getParentFile().exists()) { | ||
| file.getParentFile().mkdir(); | ||
| file.createNewFile(); | ||
| initialiseUser(); | ||
| System.out.println("B" + file.getParentFile().getPath()); | ||
| } else if (!file.exists()) { | ||
| file.createNewFile(); | ||
| initialiseUser(); | ||
| System.out.println("C" + file.getPath()); | ||
| } else { | ||
| Duke.logger.log(Level.INFO, filepath + " is found!"); | ||
| } | ||
|
|
||
| } catch (IOException e) { | ||
| System.out.println("before reader"); | ||
| throw new DukeException("Failed to create new file"); | ||
| } | ||
|
|
||
| BufferedReader reader = new BufferedReader(new FileReader(filepath)); | ||
| String line; | ||
| int count = -1; | ||
| while ((line = reader.readLine()) != null) { | ||
| if (count == -1) { | ||
| username = line; | ||
| } else { | ||
| topicsDone.add(Integer.parseInt(line)); | ||
| } | ||
| ++count; | ||
| } | ||
| reader.close(); | ||
| } catch (IOException e) { | ||
| System.out.println("after reader"); | ||
| throw new DukeException("Failed to close reader"); | ||
| } | ||
| } |
Comment on lines
34
to
51
| if (!file.getParentFile().getParentFile().exists()) { | ||
| file.getParentFile().getParentFile().mkdir(); | ||
| file.getParentFile().mkdir(); | ||
| file.createNewFile(); | ||
| initialiseUser(); | ||
| System.out.println("A" + file.getParentFile().getParentFile().getPath()); | ||
| } else if (!file.getParentFile().exists()) { | ||
| file.getParentFile().mkdir(); | ||
| file.createNewFile(); | ||
| initialiseUser(); | ||
| System.out.println("B" + file.getParentFile().getPath()); | ||
| } else if (!file.exists()) { | ||
| file.createNewFile(); | ||
| initialiseUser(); | ||
| System.out.println("C" + file.getPath()); | ||
| } else { | ||
| Duke.logger.log(Level.INFO, filepath + " is found!"); | ||
| } |
There was a problem hiding this comment.
Good effort in keeping this neat and nicely formatted. However, having some comments here would be nice to help the developer understand your code.
Comment on lines
40
to
51
| if (!tasksFile.getParentFile().getParentFile().exists()) { | ||
| tasksFile.getParentFile().getParentFile().mkdir(); | ||
| tasksFile.getParentFile().mkdir(); | ||
| tasksFile.createNewFile(); | ||
| System.out.println("A" + tasksFile.getParentFile().getParentFile().getPath()); | ||
| } else if (!tasksFile.getParentFile().exists()) { | ||
| tasksFile.getParentFile().mkdir(); | ||
| tasksFile.createNewFile(); | ||
| System.out.println("B" + tasksFile.getParentFile().getPath()); | ||
| } else if (!tasksFile.exists()) { | ||
| tasksFile.createNewFile(); | ||
| System.out.println("C" + tasksFile.getPath()); |
There was a problem hiding this comment.
This would be a good place to use a logger, as opposed to system io, since your team is already using one.
| */ | ||
| public Deadline(String description, String by) throws DukeException { | ||
| super(description); | ||
| this.by = by; |
Comment on lines
28
to
32
| try { | ||
| setFrequency(frequency); | ||
| } catch (DukeException e) { | ||
| //e.getMessage(); | ||
| } |
There was a problem hiding this comment.
We went through this in class. https://nuscs2113-ay1920s1.github.io/website/schedule/week6/tutorial-cs2113.html
+Resized window +Added some storage tests
error handling for files that are not named properly
v1.3: Implemented DeleteNoteCommand and refactored mainwindow
added 3 difficulity levels for quiz
v1.4 last min
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.
@claysmilesoil @GlenWong97 @kishore03109