[AY1920S1-CS2113-T13-3] Chronologer #34
Conversation
hanskw4267
changed the title
There was a problem hiding this comment.
As a team, please go through the comments even if it is related to one person's code. I havent repeated pointing out the same mistake. In design, I have the same confusion that I raised in UG and DG feedback. It is not very clear why there are so many classes with minor attribute variations (or without any difference). In case, If these class will have behavioural changes in future then it should be fine. Otherwise, it is pointless to create so many classes with minor/no difference.
src/main/java/DialogBox.java
Outdated
| } | ||
|
|
||
| /** | ||
| * The getUserDialog distinction is needed to enable the flipping of the labels to create the chat bot like |
There was a problem hiding this comment.
The method comment should start with a simple present. Once you explain 'what it does' in the first sentence, then you can explain 'why' or any other important information to be conveyed
src/main/java/Duke.java
Outdated
| + "|____/ \\__,_|_|\\_\\___|\n"; | ||
| System.out.println("Hello from\n" + logo); | ||
|
|
||
| private String filePath = System.getProperty("user.dir") + "/src/DukeDatabase/ArrayList"; |
There was a problem hiding this comment.
The magical strings should be defined as a constant. Even if the filePath changes somewhere in the middle of the process, the magical postfix can be defined as a constant with the name indicating it is a default one
src/main/java/Duke.java
Outdated
| } | ||
|
|
||
| /** | ||
| * This method runs the main program. |
There was a problem hiding this comment.
This should be 'Runs the main program'. I will not point out the same mistake in the method comment in the following implementation. Please make sure it is fixed everywhere.
There was a problem hiding this comment.
The class was rendered redundant, fixed this issue across the project
src/main/java/Main.java
Outdated
| FXMLLoader fxmlLoader = new FXMLLoader(Main.class.getResource("/view/MainWindow.fxml")); | ||
| AnchorPane ap = fxmlLoader.load(); | ||
| Scene scene = new Scene(ap); | ||
| stage.getIcons().add(new Image(getClass().getResourceAsStream("/images/GuiLogo.png"))); |
There was a problem hiding this comment.
This method does not flow SLAP though it is very short. If start method does a functionality at the level of calling someone else's method (e.g. initializeDukeElements() ), then it should be maintained throughout the method. But here it does a very lower-level work of creating and setting object properties
src/main/java/Main.java
Outdated
| Scene scene = new Scene(ap); | ||
| stage.getIcons().add(new Image(getClass().getResourceAsStream("/images/GuiLogo.png"))); | ||
| stage.setScene(scene); | ||
| stage.setTitle("Chronologer"); |
There was a problem hiding this comment.
Please define these magical strings as constants
| duration = ChronoUnit.HOURS.between(LocalDateTime.now(), dateList.get(i).startDate); | ||
| } | ||
| if (durationToFind <= duration) { | ||
| if (i != 0) { |
There was a problem hiding this comment.
There are three levels of if here which is very hard to read and understand. Try to simplify it
| public static String wrongDateOrTime() { | ||
| Ui.printManual(); | ||
| Ui.printDash(); | ||
| return "☹ OOPS!!! The date or time of this add type command is not of the correct format." |
| public static DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("dd/MM/yyyy HHmm"); | ||
| private static LocalDateTime dateEvent; | ||
| private static LocalDateTime dateDeadline; | ||
| private static LocalDateTime datePostpone; |
There was a problem hiding this comment.
If you want to only assign and return, what is the purpose of having this many variables of same type. If you have any future use, then it should be fine.
src/main/java/parser/Flag.java
Outdated
| BY("/by"), | ||
| AT("/at"), | ||
| BETWEEN("/between"), FOR("/for"), | ||
| IN("/in"); |
| * @throws DukeException The DukeException class has all the respective methods | ||
| * and messages! | ||
| */ | ||
| public static Command parse(String userInput) throws DukeException { |
There was a problem hiding this comment.
Too lengthy to read and follow what it does. Try to simplify this by extracting out low-level operation to methods.
…rt to. Command: export <file name>
…t date and end date.
…closer to a command line!
B refactor postpone
# Conflicts: # src/main/java/chronologer/command/ExportCommand.java
Implemented a history like feature for the chatbot.
B parser test
Added test cases and did quite a few bug fixes.
minor quality edit
updated ui.png
added commands for testing purpose
DG & UG update after module lead approval.
* test * test * test * test * test * test * test * image * image * image * image * update * update * update * update * update
@TanYiXiang
@E0310898
@fauzt
@assman
@hanskw4267