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

[Sean Ng] iP #286

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

[Sean Ng] iP #286

wants to merge 57 commits into from

Conversation

snss231
Copy link

@snss231 snss231 commented Jan 28, 2022

Duke bro

  • this
  • is
  • a
  • bullet
  • list
  1. this
  2. is
  3. a
  4. numbered
  5. list
public static void Main(String[] args) {
   System.out.println("This is a fenced code block");
}
  • this
  • is
  • a
  • task
  • list

👍 Emojis are cool

"Do. Or do not. There is no try." - Yoda

This is a hyperlink

This is inline code.

bold italic strikethrough

Copy link

@aweijun aweijun 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, learned a lot from reviewing your IP!
Some cosmetic changes that were proposed.

src/main/java/duke/Parser.java Outdated Show resolved Hide resolved
src/main/java/duke/Storage.java Outdated Show resolved Hide resolved
src/main/java/duke/Ui.java Outdated Show resolved Hide resolved
* @return The completed task.
*/
public Task mark(int index) {
Task task = this.tasks.get(index);
Copy link

Choose a reason for hiding this comment

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

I am unsure if "this" statement is necessary?

This doesn't recommend based on the coding standard.

Can considering removing this statement.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't aware of this, my perception was that the "this" keyword should be used where possible to make the code more explicit. I tried finding information related to the "this" keyword in the coding standard document @ https://se-education.org/guides/conventions/java/intermediate.html but couldn't find it, mind pointing me in the right direction? thanks 😄

Copy link

@aweijun aweijun left a comment

Choose a reason for hiding this comment

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

Great work for the IP!

src/main/java/duke/Parser.java Outdated Show resolved Hide resolved
Copy link

@clement0010 clement0010 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 follows coding standards and not many issues to nitpick. Good Job!

return isDone ? "X" : " ";
}

public boolean descriptionContains(String keyword) {

Choose a reason for hiding this comment

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

Perhaps containsDescription or hasDescription would sound better for this function?

Comment on lines 43 to 44
} catch (IndexOutOfBoundsException e) {
throw new DukeException("Invalid parameter(s). duke.task.Task " + (index + 1) + " does not exist");

Choose a reason for hiding this comment

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

I think IndexOutOfBoundsException will never be caught here? The error will be thrown in the execute function and the same error was handled there as well.

*/
public TaskList load() {
Path dataFolder = Path.of(PROJECT_ROOT, "data");
if (!Files.exists(dataFolder)) { //initialize data directory if not present

Choose a reason for hiding this comment

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

Good use of guard clause here

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.

None yet

4 participants