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.

Sign up for GitHub

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

Jump to bottom

[Cheah Hao Yi] iP #57

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

[Cheah Hao Yi] iP #57

wants to merge 42 commits into from

Conversation

CheahHaoYi
Copy link

Added week 3 iP progress

dhanish265
dhanish265 reviewed Sep 2, 2022
View reviewed changes
Copy link

@dhanish265 dhanish265 left a comment

Choose a reason for hiding this comment

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

Overall, I think it's quite good. Just need those minor edits that I suggested.

src/main/java/Duke.java Outdated
public class Duke {
public static void main(String[] args) {

public static Task[] tasks = new Task[100];
Copy link

Choose a reason for hiding this comment

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

Please replace magic literal ('100'). Consider using a final value.

src/main/java/Duke.java Outdated
public static Task[] tasks = new Task[100];
public static int numTasks = 0;

public static void listTasks(){
Copy link

Choose a reason for hiding this comment

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

I think there should be a space between the closing bracket and open brace. You can use the reformat code option for the IDE to do it for you

src/main/java/Duke.java Outdated
System.out.println("What can I do for you?");
}

public static boolean toExit(String userInput) {
Copy link

Choose a reason for hiding this comment

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

can consider changing the method name to reflect the boolean nature of the method (toExit does not sound like it's a boolean method)

src/main/java/Duke.java Outdated
System.out.println("Beep boop, now you have " + numTasks + " tasks");
}

public static void addDeadline(String param){
Copy link

Choose a reason for hiding this comment

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

perhaps it is accidental, but there are actually 2 spaces between 'public' and 'static' in this method and the next method

kaseykwok
kaseykwok reviewed Sep 2, 2022
View reviewed changes
Copy link

@kaseykwok kaseykwok left a comment

Choose a reason for hiding this comment

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

Nice work. Hope my review could help.

src/main/java/Duke.java Outdated
public class Duke {
public static void main(String[] args) {

public static Task[] tasks = new Task[100];
Copy link

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 adding a constant for it.

src/main/java/Duke.java Outdated
final String ERROR_OUT_OF_BOUND = "Sorry, the task does not seem to exist :<";
tasks[whichTask].setStatus(done);

if(whichTask > numTasks){
Copy link

Choose a reason for hiding this comment

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

Consider adding spaces before the opening parenthesis and after the closing parenthesis.

src/main/java/Duke.java Outdated
System.out.println("Beep boop, now you have " + numTasks + " tasks");
}

public static void addDeadline(String param){
Copy link

Choose a reason for hiding this comment

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

Avoid double spacing.

src/main/java/Duke.java Outdated
*/

final String LOGO = "\n" +
" _________________________________________\n" +
Copy link

Choose a reason for hiding this comment

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

Indentation for wrapped lines should be 8 spaces.

src/main/java/Duke.java Outdated

public static void listTasks(){
System.out.println("Beep beep, listing out the tasks....Loading.....");
for(int i = 0; i < numTasks; i++){
Copy link

Choose a reason for hiding this comment

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

Consider adding space before the opening parenthesis.

morbenami1
morbenami1 reviewed Sep 2, 2022
View reviewed changes
Copy link

@morbenami1 morbenami1 left a comment

Choose a reason for hiding this comment

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

Over all great job, amazing code and very creative :)

src/main/java/Duke.java Outdated
public class Duke {
public static void main(String[] args) {

public static Task[] tasks = new Task[100];
Copy link

Choose a reason for hiding this comment

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

use magic number

src/main/java/Duke.java Outdated
}

public static void processUserInput(String userInput){
String[] splitInput = userInput.split(" ", 2);
Copy link

Choose a reason for hiding this comment

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

instead of splitInput- inputSplitedBySpace

src/main/java/Task.java Outdated
private boolean isDone;
private String taskDescription;

private final String STATUS_DONE_ICON = "X";
Copy link

Choose a reason for hiding this comment

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

gread job! adding the final String for the mark status

src/main/java/Duke.java Outdated
public static void main(String[] args) {

public static Task[] tasks = new Task[100];
public static int numTasks = 0;
Copy link

Choose a reason for hiding this comment

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

change to a more informative name

sevenseasofbri
sevenseasofbri reviewed Sep 16, 2022
View reviewed changes
Copy link

@sevenseasofbri sevenseasofbri left a comment

Choose a reason for hiding this comment

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

Hi Hao Yi, good job on the iP so far! I have added some comments on the code quality, coding standard violations and naming conventions. Hope they will be helpful. 👍🏽

src/main/java/Duke/Duke.java Outdated

public static void greetUser() {

final String LOGO = "\n"
Copy link

Choose a reason for hiding this comment

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

Consider moving the final variable to make it a global variable in this class.

src/main/java/Duke/Duke.java Outdated
Comment on lines 57 to 59
System.out.println(LOGO);
System.out.println("Hello! I'm Duke");
System.out.println("What can I do for you?");
Copy link

Choose a reason for hiding this comment

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

Perhaps can avoid the usage of magic literals by declaring these as final variables in this class. (Same for line 64)

src/main/java/Duke/Duke.java Outdated
System.out.println("What can I do for you?");
}

public static boolean isToExit(String userInput) {
Copy link

Choose a reason for hiding this comment

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

Perhaps name this function isExit since it would be easier to understand at first glance.

src/main/java/Duke/InputParser.java Outdated
Comment on lines 20 to 24
case ("list"): //Fallthrough
case ("mark"): //Fallthrough
case ("unmark"): //Fallthrough
case ("todo"): //Fallthrough
case ("deadline"): //Fallthrough
Copy link

Choose a reason for hiding this comment

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

Perhaps remove the comment //Fallthrough since it is redundant. You can consider adding one fallthrough comment at the end of the case. (I see this in a couple of places, so do look into that)

src/main/java/Duke/InputParser.java Outdated
return false;
}
}
private boolean isCorrectInput(String[] parsed ) {
Copy link

Choose a reason for hiding this comment

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

Consider adding at least a single line space between functions to improve readability

src/main/java/Duke/InputParser.java Outdated
}

private String[] parseParameter(String inputString, String optionFlag){
int OPTION_LEN = 4;
Copy link

Choose a reason for hiding this comment

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

Naming conventions of regular variables is pascalCase. Consider using that here instead of all caps.

src/main/java/Duke/InputParser.java Outdated

public void parseUserInput(String userInput) throws UnknownCommandException, DukeException {

String[] inputSplitBySpace = userInput.split(" ", 2);
Copy link

Choose a reason for hiding this comment

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

Do consider avoiding the usage of magic literals. Store and use the values as final variables instead

src/main/java/Duke/InputParser.java Outdated
public void parseUserInput(String userInput) throws UnknownCommandException, DukeException {

String[] inputSplitBySpace = userInput.split(" ", 2);
//assume first word input by user is the command
Copy link

Choose a reason for hiding this comment

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

Bring this comment closer to right above the line it describes, so it is clear as to what you are talking about

src/main/java/Duke/TaskManager.java Outdated
public class TaskManager {

private static final int RESIZE_FACTOR = 2;
private static int NUM_TASK;
Copy link

Choose a reason for hiding this comment

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

Please follow the naming conventions for regular variables. It should be pascalCase for this one since it is not a final variable

@CheahHaoYi
Added save functionality
7155402
@CheahHaoYi
Merge branch 'branch-Level-6'
37a8a94 
# Conflicts:
#	src/main/java/Duke/InputParser.java
@CheahHaoYi
Merge branch 'branch-Level-7'
7a1cc50 
Create Jar file

# Conflicts:
#	src/main/java/Duke/TaskManager.java
@CheahHaoYi
Address CS2113 TA mid-term comment
667eb9a
@CheahHaoYi
Reorganize Exception classes
5ca178b 
Add new exceptions
Refactor Exception using Inheritance
@CheahHaoYi
Refactor packages
a81150f 
Init class for Level 8
@CheahHaoYi
Change utiliti classes to static references
a3d18d3 
Refactor some functions from Duke class into utilities classes
@CheahHaoYi
Fix the bug to remove redundant commands stored in data file
ff60b2d
CheahHaoYi and others added 16 commits September 30, 2022 01:55
@CheahHaoYi
Add Command class
b54fa9a 
Refactor functionalities more clearly into the utility classes
@CheahHaoYi
Attempt to use hashset to implement find functionality
7666660
@CheahHaoYi
Rename parser and tasklist
c881119 
Add exception check for wrong option flag
Add find functionality
@CheahHaoYi
Add find functionality
7f63388
@CheahHaoYi
Merge pull request #1 from CheahHaoYi/branch-Level-9
5bf5380 
Level 9, Week 7 iP task: Find
@CheahHaoYi
Add JavaDoc
bee95f8
@CheahHaoYi
Add help functionality, user guide draft
12fbdeb
@CheahHaoYi
Polish up user guide, Fix ArrayOutOfBoudException for Delete
7ece7bf
@CheahHaoYi
Update test input and expected output
6059848
@CheahHaoYi
Minor Edits to JavaDoc
be01be8 
To push A-MoreOOP tag onto github (missed out earlier)
@CheahHaoYi
Fix minor coding style violations
ee9bc5d 
To create branch for A-JavaDoc (missed out earlier)
@CheahHaoYi
Merge pull request #2 from CheahHaoYi/branch-A-JavaDoc
82cb255 
Fix minor coding style violations
@CheahHaoYi
update readme.md
ba2c021
@CheahHaoYi
fix format error on Readme
d46a13f
@CheahHaoYi
Fix check bug
b2d6236
@CheahHaoYi
Fix parser bug for delete, mark, unmark
659bfdf 
Fix parser bug for bad option flags
Refactor to replace hardcoded line seperator with java system line separator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhanish265 dhanish265 dhanish265 left review comments

@sevenseasofbri sevenseasofbri sevenseasofbri left review comments

@kaseykwok kaseykwok kaseykwok left review comments

@morbenami1 morbenami1 morbenami1 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@CheahHaoYi @dhanish265 @sevenseasofbri @kaseykwok @morbenami1