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.

Sign up for GitHub

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

Jump to bottom

imaginarys96 iP #73

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

imaginarys96 iP #73

wants to merge 29 commits into from

Conversation

imaginarys96
Copy link

No description provided.

spinoandraptos
spinoandraptos reviewed Sep 6, 2023
View reviewed changes
src/main/java/Task.java Outdated
@@ -1,10 +1,11 @@
public class Task {
protected String description;
protected boolean isDone;

public Task(String description) {
protected String type;
Copy link

Choose a reason for hiding this comment

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

type is ambiguous, can be mistaken as a verb, recommend to use taskType

src/main/java/Chatbot.java Outdated
}
System.out.println("____________________________________________________________");
} else if ( input.startsWith("mark ") ) {
} else if (input.startsWith("mark ")) {
String number = input.replace("mark ", "").trim();
Copy link

Choose a reason for hiding this comment

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

number is ambiguous, recommend to use taskNumber

src/main/java/Chatbot.java Outdated
}
System.out.println("____________________________________________________________");
} else if ( input.startsWith("mark ") ) {
} else if (input.startsWith("mark ")) {
String number = input.replace("mark ", "").trim();
int markTaskNo = Integer.parseInt(number);
Copy link

Choose a reason for hiding this comment

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

noun should not be named with a verb in it, recommend to combine with previous line and use taskNumber as previously mentioned

src/main/java/Chatbot.java Outdated
if( markTaskNo > 0 ) {
tasks[markTaskNo-1].markAsDone();
if (markTaskNo > 0) {
tasks[markTaskNo - 1].markAsDone();
Copy link

Choose a reason for hiding this comment

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

using -1 magical number, would be good to define what the -1 means

src/main/java/Chatbot.java Outdated
System.out.println("____________________________________________________________");
} else if ( input.startsWith("unmark ") ) {
} else if (input.startsWith("unmark ")) {
String number = input.replace("unmark ", "").trim();
int unmarkTaskNo = Integer.parseInt(number);
Copy link

Choose a reason for hiding this comment

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

variable name can be mistaken as an action, recommend to use unmarkedTaskNo

src/main/java/Chatbot.java Outdated
System.out.println( " [" + tasks[unmarkTaskNo-1].getStatusIcon() + "] " + tasks[unmarkTaskNo-1].getDescription() );
System.out.println(" [" + tasks[unmarkTaskNo - 1].getStatusIcon() + "] " + tasks[unmarkTaskNo - 1].getDescription());
System.out.println("____________________________________________________________");
} else if ( input.startsWith("todo") ) {
Copy link

Choose a reason for hiding this comment

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

unnecessary spaces for start and ends of brackets

src/main/java/Task.java Outdated
@@ -19,4 +20,6 @@ public void markAsUndone() {
this.isDone = false;
}

public String getTypeIcon() { return type; }
Copy link

Choose a reason for hiding this comment

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

curly bracket should start on a newline

Musfirahe0556596
Musfirahe0556596 reviewed Sep 16, 2023
View reviewed changes
Copy link

@Musfirahe0556596 Musfirahe0556596 left a comment

Choose a reason for hiding this comment

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

Overall good job but some coding quality and standards to follow!

src/main/java/chatbot/Chatbot.java Outdated
@@ -0,0 +1,128 @@
package chatbot;

import chatbot.*;
Copy link

Choose a reason for hiding this comment

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

Imported classes should always be listed explicitly (avoid *)

src/main/java/chatbot/Chatbot.java Outdated

public class Chatbot {

public static void main(String[] args) throws Exception {
Copy link

Choose a reason for hiding this comment

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

Avoid long methods of more than 30 LoC

src/main/java/chatbot/Chatbot.java Outdated
System.out.println("____________________________________________________________");
System.out.println(" Here are the tasks in your list:");
for (int i = 0; i < numOfTasks; i++) {
//System.out.println(" " + (i + 1) + ".[" + tasks[i].getTypeIcon() + "][" + tasks[i].getStatusIcon() + "] " + tasks[i].getDescription());
Copy link

Choose a reason for hiding this comment

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

Remove commented code

src/main/java/chatbot/Chatbot.java Outdated
System.out.println(" [" + tasks[unmarkedTaskNo - 1].getStatusIcon() + "] " + tasks[unmarkedTaskNo - 1].getDescription());
System.out.println("____________________________________________________________");
} else if (input.startsWith("todo")) {
String msg = input.replace("todo", "").trim();
Copy link

Choose a reason for hiding this comment

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

Avoid foreign language words, slang, and names (msg)

src/main/java/chatbot/Chatbot.java Outdated
System.out.println(" Now you have " + String.valueOf(numOfTasks) + " tasks in the list.");
System.out.println("____________________________________________________________");
} else if (input.startsWith("deadline")) {
String msg = input.replace("deadline", "").trim();
Copy link

Choose a reason for hiding this comment

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

Avoid foreign language words, slang, and names (msg)

src/main/java/chatbot/Chatbot.java Outdated
}

String byDate = msg.substring(msg.indexOf("/by ") + 4).trim(); // will contain the byDate
String desc = msg.substring(0, msg.indexOf("/by")); // will contain the deadline description
Copy link

Choose a reason for hiding this comment

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

Avoid foreign language words, slang, and names (desc)

src/main/java/chatbot/Chatbot.java Outdated
System.out.println(" Now you have " + String.valueOf(numOfTasks) + " tasks in the list.");
System.out.println("____________________________________________________________");
} else if (input.startsWith("event")) {
String msg = input.replace("event", "").trim();
Copy link

Choose a reason for hiding this comment

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

Avoid foreign language words, slang, and names (msg)

src/main/java/chatbot/Chatbot.java Outdated
String dateRange = msg.substring(msg.indexOf("/from ") + 6).trim();
String fromDate = dateRange.substring(0, dateRange.indexOf("/to ")).trim();
String toDate = dateRange.substring(dateRange.indexOf("/to ") + 4).trim();
String desc = msg.substring(0, msg.indexOf("/from ")); // will contain the deadline description
Copy link

Choose a reason for hiding this comment

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

Avoid foreign language words, slang, and names (desc)

farissirraj pushed a commit to farissirraj/ip that referenced this pull request Sep 22, 2023
@seanleong339
.gitignore: Fix ACTUAL.txt -> ACTUAL.TXT (nus-cs2113-AY2324S1#73)
6840ef7 
text-ui-test/runtest.bat|sh scripts generate a file ACTUAL.TXT.
However, .gitignore uses ACTUAL.txt, which means the generated
file will not be ignored by Git on non-Windows OS.

Let's update .gitignore as ACTUAL.txt -> ACTUAL.TXT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spinoandraptos spinoandraptos spinoandraptos left review comments

@Musfirahe0556596 Musfirahe0556596 Musfirahe0556596 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@imaginarys96 @Musfirahe0556596 @spinoandraptos @imaginarys996