-
Notifications
You must be signed in to change notification settings - Fork 77
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
[ishitamandal06] iP #71
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat code, good job!
src/main/java/Duke.java
Outdated
System.out.println("\t"+(i+1)+". "+ inputs.get(i) ); | ||
} | ||
System.out.println("____________________________________________________________"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the ' else{ ' line should be on this line instead of the next line
src/main/java/Duke.java
Outdated
for(int i=0;i< inputs.size();i++){ | ||
System.out.println("\t"+(i+1)+". "+ inputs.get(i) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to include one space character for example "int i = 0 ; i < inputs.size() ; i++"
src/main/java/Duke.java
Outdated
for(int i=0;i< inputs.size();i++){ | ||
System.out.println("\t"+(i+1)+". "+ inputs.get(i) ); | ||
} | ||
System.out.println("____________________________________________________________"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line makes the code look long and messy, maybe can be replaced with a helper function defined above
src/main/java/Duke.java
Outdated
String inputText = ""; | ||
List<String> inputs = new ArrayList<>(); | ||
boolean isRunning = true; | ||
while(isRunning){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spaces can be put in between like so: "while (isRunning) { "
"\tadded: "+inputText +"\n"+ | ||
"____________________________________________________________\n"); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary empty line here
src/main/java/Duke.java
Outdated
else{ | ||
inputs.add(inputText); | ||
System.out.println("____________________________________________________________\n" + | ||
"\tadded: "+inputText +"\n"+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good that the string is segmented to avoid overly long lines of code. however, indents can be made to align the string to make it more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good effort! Please consider making the entire code more readable with more SLAP, following the coding standards (take note of spacings) and avoiding complicated expressions as well as magic numbers.
@@ -0,0 +1,14 @@ | |||
public class Deadline extends Task{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code does not comply with coding standards, perhaps you can consider adding space between Task
and {
.
src/main/java/Duke.java
Outdated
} else if (arr[0].equals("deadline")) { | ||
tasks[taskCount++] = new Deadline(inputText.substring(9,inputText.indexOf('/')),inputText.substring(inputText.indexOf('/')+4)); | ||
}else { | ||
tasks[taskCount++] = new Event(inputText.substring(6,inputText.indexOf('/')),inputText.substring(inputText.indexOf('/')+4)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid complicated expressions. Consider abstracting the details into other functions or break down the logic into mutiple statements. Please take note of the length of the code and avoid long methods.
src/main/java/Duke.java
Outdated
int position = Integer.parseInt(inputText.substring(inputText.length()-1)); | ||
if ((inputText.substring(0,4).equals("mark"))){ | ||
tasks[position-1].markAsDone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using magic literals. Consider putting the numbers into variables to make the code more easily understood. For example, 0
can be put iinto a variable STARTING_INDEX_OF_COMMAND_STRING
.
src/main/java/Duke.java
Outdated
tasks[position-1].markAsDone(); | ||
System.out.println("Nice! I've marked this task as done:"); | ||
|
||
}else{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code does not comply with the coding standards. Please consider adding spaces between }
, else
and {
.
src/main/java/Duke.java
Outdated
if(inputText.equals("bye")) { | ||
isRunning = false; | ||
showExitMessage(); | ||
} else if (inputText.equals("list")) { | ||
handleListMessage(); | ||
}else if ((inputText.substring(0,4).equals("mark")) || inputText.substring(0,6).equals("unmark") ) { | ||
handleMarkMessage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code can be further improved with more abstraction (SLAP).
} | ||
|
||
public String getStatusIcon() { | ||
return (isDone ? "X" : " "); // mark done task with X |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid complicated expressions, consider seperating the logic to make the code more understandable. Also remember to remove uneeded comments after making the statements clear (i.e. comments should avoid stating the obvious).
return "[" + this.getStatusIcon()+"] " + this.description; | ||
} | ||
|
||
//... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider removing this comment because it serves no purpose.
No description provided.