Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tuan Le] Duke Increments #143

Open
wants to merge 63 commits into
base: master
Choose a base branch
from
Open

[Tuan Le] Duke Increments #143

wants to merge 63 commits into from

Conversation

youaremysky99
Copy link

No description provided.

j-lum and others added 30 commits August 6, 2019 15:25
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (#9)
Gradle defaults to an empty stdin which results in runtime exceptions
when attempting to read from `System.in`. Let's add some sensible
defaults for students who may still need to work with the standard
input stream.
Add configuration for console applications
The OpenJFX plugin expects applications to be modular and bundled
with jlink, resulting in fat jars that are not cross-platform. Let's
manually include the required dependencies so that shadow can package
them properly.
Copy link

@khsc96 khsc96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall nice use of OOP. Code is generally clean and readable except for some indentations problem.

@@ -0,0 +1,80 @@
package duke;
import java.util.List;
import duke.task.*;
Copy link

@khsc96 khsc96 Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps what you can do to prevent intellij from auto importing * is to change the settings in intellij by going to
File > settings > code style > java > imports > change the following:
Class count to use import *;
Names count to use static import with *;
to some large enough number

Comment on lines 3 to 10
import duke.Storage;
import duke.TaskList;
import duke.dukeException.DukeParseException;
import java.util.ArrayList;
import java.util.List;
import duke.Interpreter;
import duke.task.Task;
import duke.DukeResponse;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can consider grouping import with duke package together and separate it out with java.util package for better readability

Comment on lines 1 to 31
import java.util.Scanner;
import java.util.List;
import java.util.ArrayList;
import duke.task.*;
import java.io.IOException;
import java.io.BufferedWriter;
import java.io.FileWriter;
import java.io.File;
import duke.dukeException.DukeParseException;
import duke.Interpreter;
import duke.Storage;
import duke.TaskList;
import duke.Parser;
import duke.commands.Command;
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.Label;
import javafx.stage.Stage;
import javafx.scene.control.Button;
import javafx.scene.control.ScrollPane;
import javafx.scene.control.TextField;
import javafx.scene.layout.AnchorPane;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
import javafx.scene.layout.Region;
import javafx.scene.control.Label;
import duke.DialogBox;
import javafx.scene.image.Image;
import javafx.scene.image.ImageView;
import javafx.application.Platform;
import duke.DukeResponse;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can consider regrouping this part too. Also the * import.

Comment on lines 34 to 40
static final String separation = "_________________________________________________";
static final String greetingMessage = "Salue! Je suis Duke. \nWhat can I do for you?";
static final String pathToData = "data/storage.txt";
private Storage storage;
private TaskList taskList;
private Parser parser;
private ScrollPane scrollPane;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may want to change indentation to 4 spaces instead to suit coding standard

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the final values should be named in capital letters.

* the dialog container. Clears the user input after processing.
*/
private void handleUserInput() {
String userCommand = userInput.getText();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too have a weird indentation added, might want to remove it

import duke.task.*;
import duke.commands.*;

public class Parser {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps can consider using your enum class here and use switch case instead

Comment on lines +52 to +59
switch (type) {
case "todo":
containers.add(new ToDo(record));
break;
case "deadline":
containers.add(new Deadline(record));
break;
case "event":
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can perhaps use your enum class here too

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good choice to increase readability for your codes.

Comment on lines +55 to +64
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append(" [D]")
.append(super.getStatusIcon())
.append(" ")
.append(super.description)
.append(" (by ")
.append(this.date.format(DateTimeFormatter.ofLocalizedDate(FormatStyle.FULL)))
.append(")");
return sb.toString();
Copy link

@khsc96 khsc96 Feb 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice use of indentations here, code is clean and very readable.
But I think you can check your tab key is it changed to 4 spaces instead of tab in intellij, your spaces looks like it is not 4 spaces.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree.

@@ -0,0 +1,8 @@
package duke.task;

public enum TaskType {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps can consider removing this enum class, does not seems like it is being used.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you can think about javaDoc as well here.

Copy link

@bluesky0911 bluesky0911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, well written. good job! however, you can think about javaDoc plus naming issues. For example, naming for final variables, some methods starting with capital latter, etc. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants