Skip to content

[Ee Hong Zhi] ip#78

Open
Hongzhii wants to merge 22 commits into
nus-cs2113-AY2324S1:masterfrom
Hongzhii:master
Open

[Ee Hong Zhi] ip#78
Hongzhii wants to merge 22 commits into
nus-cs2113-AY2324S1:masterfrom
Hongzhii:master

Conversation

@Hongzhii

@Hongzhii Hongzhii commented Sep 6, 2023

Copy link
Copy Markdown

No description provided.

@skylee03 skylee03 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aside from violating coding guidelines a bit, well done!

@@ -0,0 +1,19 @@
public class Deadline extends Task{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public class Deadline extends Task{
public class Deadline extends Task {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a space before the curly bracket is more consistent with our coding conventions.

Comment thread src/main/java/Deadline.java Outdated
@@ -0,0 +1,19 @@
public class Deadline extends Task{
private String deadline;
public Deadline(String description, String deadline){

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public Deadline(String description, String deadline){
public Deadline(String description, String deadline) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a space before the curly bracket is more consistent with our coding conventions.

}

@Override
public void show(){

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public void show(){
public void show() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a space before the curly bracket is more consistent with our coding conventions.

Comment on lines +13 to +14
}
else{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
}
else{
} else {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to our coding standards, it is recommended to write the else branch like this.

@Override
public void show(){
System.out.print("[D][");
if(isDone){

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if(isDone){
if (isDone) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a space before the curly bracket is more consistent with our coding conventions. It is also suggested to add a space after if.

Comment thread src/main/java/Duke.java Outdated

else if(keyword.equals("unmark")){
// Check exception: number of words is not 2
if(words.length != 2){

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if(words.length != 2){
if (words.length != 2) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a space before the curly bracket is more consistent with our coding conventions.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +58 to +60
}

else if(keyword.equals("unmark")){

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
}
else if(keyword.equals("unmark")){
} else if (keyword.equals("unmark")) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

According to our coding standards, it is recommended to write the else branch like this.

Comment thread src/main/java/Event.java Outdated
Comment on lines +12 to +14
public void show(){
System.out.print("[D][");
if(isDone){

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public void show(){
System.out.print("[D][");
if(isDone){
public void show() {
System.out.print("[D][");
if (isDone) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a space before the curly bracket is more consistent with our coding conventions.

Comment thread src/main/java/Duke.java Outdated
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
static private Task[] taskList = new Task[100];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
static private Task[] taskList = new Task[100];
static private Task[] tasks = new Task[100];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Plural form should be used on names representing a collection of objects.

Comment thread src/main/java/Duke.java Outdated
System.out.println("Please enter with correct format: mark [Integer]");
}
// Check exception: second word cannot be converted to integer or integer out of bounds
try{

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
try{
try {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Adding a space before the curly bracket is more consistent with our coding conventions.

@irving11119 irving11119 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.

Generally, your naming conventions are fine and adhere to the coding standards. However, do remember when using braces followed by parenthesis to leave a space in between for proper formatting.

e.g. public void method() {

Comment thread src/main/java/Deadline.java Outdated
@@ -0,0 +1,19 @@
public class Deadline extends Task{
private String deadline;
public Deadline(String description, String deadline){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
public Deadline(String description, String deadline){
public Deadline(String description, String deadline) {

Similar to most of your methods, do remember to leave a space between the closing parenthesis and the opening curly brace.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +45 to +57
try{
int markIndex = Integer.parseInt(words[1]);

if(markIndex < 1 || markIndex > numTasks){
System.out.println("Please enter a positive integer less than or equal to current number of tasks (" + numTasks + ")");
continue;
}

taskList[markIndex - 1].mark();
}
catch(NumberFormatException e){
System.out.println("Please enter with correct format: mark [Integer]");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try to avoid deep nesting. Instead of deep nesting, you could break down your statements into additional methods.

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.

3 participants