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

[Rachel Gina] iP #254

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

Conversation

rgabelarde
Copy link

No description provided.

damithc and others added 30 commits July 23, 2020 23:27
…changes. Load the data from the hard disk when the programme starts up.
…changes. Load the data from the hard disk when bot starts up.
…tput (dd-MM-yyyy to dd MMM yyyy)

# Conflicts:
#	src/Deadline.java
#	src/Duke.java
…P), add continuation of bot after exceptions caught
Copy link

@Michaeliaaa Michaeliaaa left a comment

Choose a reason for hiding this comment

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

Overall, I feel that you did an amazing job in following the coding standards! Naming wise, I think it might be better if you can give a more meaningful and clearer name to the variables rather than just letters like s, e and etc... But still, I can understand what the variables mean so I didn't comment on it.

private void addToTaskList(String[] s, ArrayList<Task> tasks) {
boolean isDone = s[2].equals("\u2713");
switch (s[0]) {
case "T":

Choose a reason for hiding this comment

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

There should be no indentation for case clauses.

Copy link

@jeffreytjs jeffreytjs left a comment

Choose a reason for hiding this comment

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

Overall, it was enjoyable to read your code as they are very clear with minor inconsistencies which could be due to the progressive improvement we make to our codes. Only very minor changes to be made.

private void addToTaskList(String[] s, ArrayList<Task> tasks) {
boolean isDone = s[2].equals("\u2713");
switch (s[0]) {
case "T":

Choose a reason for hiding this comment

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

There should be no indentation for case clauses for CS2103 convention.


public class Ui {
private Scanner userInput;
private String lastError;

Choose a reason for hiding this comment

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

Minor issue, unused variable.

import duke.task.Deadline;
import duke.task.Event;
import duke.task.Task;
import duke.task.Todo;

Choose a reason for hiding this comment

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

I like how the import statements are organised based on their association 👍

@@ -0,0 +1,119 @@
package duke.ui;
import duke.logic.TaskList;

Choose a reason for hiding this comment

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

I notice other files have the line to separate package and import statements except for this. Would be good for readability to standardize according to src/main/java/duke/logic/Storage.java

Choose a reason for hiding this comment

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

It could be the files that were written earlier that result in the inconsistent ordering of import statements.

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

3 participants