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

[SaiChaitanya13] ip #203

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

Conversation

SaiChaitanya13
Copy link

No description provided.

Copy link

@choongzhanhong choongzhanhong left a comment

Choose a reason for hiding this comment

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

Looks good!
Mainly, with regards to code style, I have pointed out some areas regarding spacing and comments that you may find could be addressed.

currentPosition++;
}*/

else{ //todo or deadline or event --> put together so don't have to repeat the same code thrice

Choose a reason for hiding this comment

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

I like the fact that you split the if/else blocks quite meaningfully, along with descriptive comments.

// filter the description and date
String taskWithDate = taskBeingAdded.description;
int indexOfSlash = taskWithDate.indexOf('/');
String taskDescription = taskWithDate.substring(0, (indexOfSlash - 1)); // substring goes to the one before the second index!!!!

Choose a reason for hiding this comment

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

Perhaps it might be good to indent the multiple line comments to be aligned with the block?

Comment on lines 8 to 11
String greeting = "Hello there! I'm Buddy\n"
+ "How may I assist you?";
String exitMessage = "Hope I was of help to you! Have a great day and see you again, Buddy :)";
String divider = "________________________________________________________________________________";

Choose a reason for hiding this comment

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

I like that you extracted these as properties. If they're constants, could you give the proper modifiers and variable name as well?

Comment on lines 115 to 118



System.out.println(divider);

Choose a reason for hiding this comment

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

Could there be fewer new lines here?

Comment on lines 22 to 23
@Override
public String toString() { // overrides --> print task prints this!

Choose a reason for hiding this comment

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

Could the comment be indented/spaced closer to the block it's referring to?

Comment on lines 23 to 26
while (! command.equals("bye")){
int index = 1;
if (command.equals("list")){
for (int i = 0; i < currentPosition; i++){ // while not null

Choose a reason for hiding this comment

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

Perhaps a space could be added before the opening braces, like so: else {?
I believe there are multiple occurrences below as well.

Copy link

@ysl-28 ysl-28 left a comment

Choose a reason for hiding this comment

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

Overall, the code is generally readable. However, the use of magic numbers could be reduced.

}

else if (command.startsWith("mark")){ // .startsWith(" ")
int taskNumber = Integer.parseInt(command.substring(5));
Copy link

Choose a reason for hiding this comment

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

Would it be clearer to use a constant instead of a magic number?

Comment on lines 54 to 61
/*else { // adding tasks
listOfThings[currentPosition] = new Task(command); // have to write in
//Task t = new Task(command);
System.out.println(divider);
System.out.println("added: " + command);
System.out.println(divider);
currentPosition++;
}*/
Copy link

Choose a reason for hiding this comment

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

Perhaps it would be more readable to remove chunks of commented code?

System.out.println(divider);
}

else if (command.startsWith("unmark")){
Copy link

Choose a reason for hiding this comment

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

Perhaps it would be clearer to follow the Java coding standard for writing if-else statements?

if (condition) {
    statements;
} else if (condition) {
    statements;
} else {
    statements;
}


else if (command.startsWith("deadline")){
Task taskBeingAdded = new Task(command.substring(9)); // task + date + slash (Description)
// filter the description and date
Copy link

Choose a reason for hiding this comment

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

I like the use of comments to explain the code when needed.

Comment on lines 67 to 68
Todo todoBeingAdded = new Todo(command.substring(5));
listOfThings[currentPosition] = todoBeingAdded;
Copy link

Choose a reason for hiding this comment

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

I like that the variable names are clear and follow the coding standard.

Copy link

@Geeeetyx Geeeetyx 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 work, but you can tidy up the code to make it look like nicer.

else if (command.startsWith("mark")){ // .startsWith(" ")
int taskNumber = Integer.parseInt(command.substring(5));
// have to parse
Task currentTask = listOfThings[taskNumber - 1];
Copy link

Choose a reason for hiding this comment

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

Good that variable names follow camel case

System.out.println(divider);
}

else if (command.startsWith("unmark")){
Copy link

Choose a reason for hiding this comment

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

Might be better to use switch case statements here

}
}

else if (command.startsWith("mark")){ // .startsWith(" ")
Copy link

Choose a reason for hiding this comment

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

It might be good to refactor these else if statements to be methods

else if (command.startsWith("deadline")){
Task taskBeingAdded = new Task(command.substring(9)); // task + date + slash (Description)
// filter the description and date
String taskWithDate = taskBeingAdded.description;
Copy link

Choose a reason for hiding this comment

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

Clear variable names that are easy to understand

System.out.println(greeting);
System.out.println(divider);

Task[] listOfThings = new Task[100]; // why cannot private static?
Copy link

Choose a reason for hiding this comment

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

Perhaps remove comments that are not explaining the code

Copy link

@maanyos maanyos left a comment

Choose a reason for hiding this comment

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

please work on the object orientedness in the code.
try to create command class (and subclasses) to refactor logic by different commands. refer to addressbook3 if necessary


static final int TOTAL_TASKS = 100;

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

Choose a reason for hiding this comment

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

too much happening in main, try to refactor to improve readibility and abstraction

Comment on lines 23 to 24
Task[] listOfThings = new Task[TOTAL_TASKS]; // why cannot private static?
int currentPosition = 0; // why cannot private static?
Copy link

Choose a reason for hiding this comment

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

don't leave comments for yourself in the code

return (isDone ? "X" : " "); // mark done task with X
}

public boolean isDone() { // do we need this
Copy link

Choose a reason for hiding this comment

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

use action naming for method

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