Skip to content

[Wang Huilin]ip#75

Open
hlwang56 wants to merge 55 commits into
nus-cs2113-AY2122S2:masterfrom
hlwang56:master
Open

[Wang Huilin]ip#75
hlwang56 wants to merge 55 commits into
nus-cs2113-AY2122S2:masterfrom
hlwang56:master

Conversation

@hlwang56

@hlwang56 hlwang56 commented Feb 2, 2022

Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/Chatbox.java Outdated
Comment on lines +1 to +19
public class Chatbox {
private String content = "";
private final String horizontalLine = "-------------------------------------";

public void chatboxPrinter(){
System.out.println(content);
System.out.println(horizontalLine);
}

public void linePrinter(){
System.out.println(horizontalLine);
}

public void setContent(String input){
this.content = input;
}


}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This class could be combined with controller.

@emilysim00 emilysim00 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your code is very well written and neat!

Comment thread src/main/java/Controller.java Outdated
chatbox.chatboxPrinter();
}

public void listen() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the method name can be more descriptive such as listenForInput so it can be better understandable.

Comment thread src/main/java/Duke.java
Comment on lines +4 to +8
Controller bot = new Controller();
bot.greet();
while(true){
bot.listen();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like the idea of having the controller.java in a separate file and not a whole chunk of code in the main program! Very Nice!

Comment thread src/main/java/TaskManager.java Outdated
Comment on lines +2 to +3
public class TaskManager {
private static final ArrayList<Task> Tasks = new ArrayList<Task>();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The use of ArrayList object is good! This helps to eliminate problem such as magic numbers or the problem of worrying the size of the list

Comment thread src/main/java/TaskManager.java Outdated
Deadline newDeadline = new Deadline(name, by);
Tasks.add(newDeadline);
int s = Tasks.size();
String response = addResponse + newDeadline.getListName() + "\n" + "Now you have " + String.valueOf(s) + " tasks in your list.";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use line-wrapping if appropriate as line length should not be more than 120 characters. The recommended line length is 110 characters.

@eigne eigne left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

With more documentation and some changes it should be good to merge.

Comment thread src/main/java/Controller.java Outdated
import java.util.Scanner;

public class Controller {
public String hello = "Hello! I'm Duke :P\nWhat can I do for you?";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will these Strings change as the program runs? If not, consider making them final and changing the variable names to uppercase.

Comment thread src/main/java/Controller.java Outdated
this.recvMsg = msg.nextLine();
String[] keyword = this.recvMsg.toLowerCase(Locale.ROOT).split(" ");
switch (keyword[0]) {
case "bye":

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider replacing these string literals with final variables in uppercase.

Comment thread src/main/java/Task.java Outdated
public void setListName(){
if(isDone == false){
this.listName = this.unmarkedStatus + this.taskName;
}else{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider adding a space before "else" to improve readability.

Comment thread src/main/java/TaskManager.java Outdated
chatbox.chatboxPrinter();
}

public void addDeadline(String name, String by){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider adding documentation to non-trivial functions.

Comment thread src/main/java/TaskManager.java Outdated
Comment on lines +44 to +54
content = "Sorry, there's no task in the list :(";
}else {
for (int i = 0; i < Tasks.size(); i++) {
String index = String.valueOf(i + 1);
String name = index + ". " + Tasks.get(i).getListName();
content += name;
if(i < Tasks.size() - 1){
content += "\n";
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks like arrowhead code. Consider moving some of this code into another method to follow SLAP

Comment thread src/main/java/Controller.java Outdated
this.addTask(this.recvMsg);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider removing this blank line.

@Ch40gRv1-Mu Ch40gRv1-Mu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good indentation!

@hlwang56

hlwang56 commented Feb 7, 2022

Copy link
Copy Markdown
Author

Thank you very much for your suggestions @matheril , @emilysim00 and @Ch40gRv1-Mu ! I'll add them on and improve the code in later commits. :D

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.

5 participants