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

[Goh Yijie Jonathan] ip #499

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

Conversation

jgyj123
Copy link

@jgyj123 jgyj123 commented Aug 28, 2022

Project Duke

  • Swift command line interface
  • good user experience
  • best robot out there 👍

Some of the core features include:

  1. Keeping track of tasks using list
  2. Marking them as complete
  3. Storing of tasks
  • Completed task
  • Do homework
  • Submit the assignment (by 20 Aug 2022)

This is the best robot I have ever met
This robot was built by Jonathan

Here is some random code:

public class Main {
    public static void main(String[] args) {
        Application.launch(MainApp.class, args);
    }
}

* @param s Command containing task message and time
* @param isCompleted whether the task is completed
*/
public Deadline (String s , boolean isCompleted) {

Choose a reason for hiding this comment

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

Should this whitespace be removed?

@@ -1,10 +1,51 @@
/**
* Main java class to create a new Duke object

Choose a reason for hiding this comment

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

according to the module's coding standard, javadoc comments should end with a fullstop. This is different from git subject headers which states not to end with a fullstop.

}
if (taskAdded) {
Task addedTask = tasks.get(tasks.getSize() - 1);
System.out.println("Got it. I've added this task: \n" + addedTask.toString() +

Choose a reason for hiding this comment

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

perhaps the + would be nicer at the start of the next line:

System.out.println("Got it. I've added this task: \n" + addedTask.toString()
        + "\nNow you have " +  tasks.getSize() +  " tasks in the list.");

public class Parser {
private Storage s;
private Ui ui;
private TaskList tasks;
Copy link

@malwaregarry malwaregarry Aug 29, 2022

Choose a reason for hiding this comment

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

perhaps a better variable would be taskList as tasks might imply a collection of task

@@ -0,0 +1,34 @@
/**
* Represents an Event that can be created by the user
*/

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 just before the class

if (taskType.equals("T")) {
tasks.add(new Todo("todo " + taskMessage, isTaskCompleted));
}
else if (taskType.equals("E")) {

Choose a reason for hiding this comment

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

perhaps to you could start the else clause to the curly bracket to meet the coding standard?

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

}

/**
* functional method to delete a task

Choose a reason for hiding this comment

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

I believe the first letter in each javadoc comment should be capitalised?

Copy link

@malwaregarry malwaregarry left a comment

Choose a reason for hiding this comment

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

LGTM not much issues. Your code is easy to read and I could only spot some minor coding standard violations.

jgyj123 and others added 14 commits August 29, 2022 23:57
There is a need to check that methods in TaskList and Storage class are being passed valid arguments

Let's use assertions to implement these safety checks

Using assertions will help to reduce bugs in the code and increase confidence in the correctness of the code
Parse method is too long and hard to read

Let's refactor the parse method

This will help to make the code more readable and scalable
Use Assert feature

There is a need to check that methods in TaskList and Storage class are being passed valid arguments

Let's use assertions to implement these safety checks

Using assertions will help to reduce bugs in the code and increase confidence in the correctness of the code
* master:
  Use Assert feature
Improve code quality of code in parser class

Parse method is too long and hard to read

Let's refactor the parse method

This will help to make the code more readable and scalable
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.

2 participants