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

[Owen] iP #107

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

[Owen] iP #107

wants to merge 53 commits into from

Conversation

blank-bank
Copy link

No description provided.

Copy link

@rafaelperes rafaelperes left a comment

Choose a reason for hiding this comment

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

Good Job Owen! Maybe you should explore ways to improve your code abstractions.

@Override
public void print(){
if (this.isDone) {
System.out.println("[D][\u2713] " + description + " (by: " + by + ")");
Copy link

@rafaelperes rafaelperes Feb 21, 2021

Choose a reason for hiding this comment

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

maybe you should avoid the need of magic strings here...

}
@Override
public String getType() {
return "E";

Choose a reason for hiding this comment

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

refer to the magic strings comment.

private static int numOfTasks = 0;
protected String description;
protected boolean isDone;
protected static ArrayList<Task> list = new ArrayList<Task>();

Choose a reason for hiding this comment

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

maybe this variable should be called listOfTasks?

this.description = description;
}
public String getType() {
return "T";

Choose a reason for hiding this comment

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

refer to the magic strings comment.

ArrayList<Task> buffer = Task.getList();
switch (buffer.get(i).getType()) {

case ("T"):
Copy link

@rafaelperes rafaelperes Feb 21, 2021

Choose a reason for hiding this comment

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

refer to the magic strings comment (maybe use enums here?).

switch (buffer.get(i).getType()) {

case ("T"):
fw.write(buffer.get(i).getType() + "\t" + buffer.get(i).isDone() + "\t" + buffer.get(i).getDescription() + "\n");

Choose a reason for hiding this comment

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

maybe you should consider improve your code abstraction here. Please check SLAP hard for example. You have fw.write repeated for each case statement.

}

public static void loadTask(String line){
String[] arr = line.split("\t");

Choose a reason for hiding this comment

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

maybe assign arr[0],arr[1],arr[2],arr[3] variable names in order to improve the code's readability?

writeToFile();
return -1;

case ("list"):

Choose a reason for hiding this comment

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

refer to the magic string comment.

}
System.out.println("Got it. I've added this task: ");
String item = line.substring(indexOfSpace + 1, indexOfSlash - 1);
String extra = line.substring(indexOfSlash + 4);

Choose a reason for hiding this comment

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

maybe we should have some code comments here explaining the logic behind +1,-1,+4?

blank-bank and others added 10 commits February 23, 2021 09:37
# Conflicts:
#	list.txt
#	src/main/java/duke/items/Event.java
#	src/main/java/duke/items/Task.java
#	src/main/java/duke/main/Parser.java
#	src/main/java/duke/main/UI.java
# Conflicts:
#	list.txt
#	src/main/java/duke/items/Event.java
#	src/main/java/duke/items/Task.java
#	src/main/java/duke/main/Parser.java
#	src/main/java/duke/main/UI.java
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.

2 participants