-
Notifications
You must be signed in to change notification settings - Fork 187
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
[Rawal Aarav] iP #172
base: master
Are you sure you want to change the base?
[Rawal Aarav] iP #172
Conversation
src/main/java/G_one.java
Outdated
import java.util.List; | ||
import java.util.Scanner; | ||
|
||
public class G_one { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the public class G_one should be named using PacalCase ( gOne/GOne)
src/main/java/G_one.java
Outdated
@@ -0,0 +1,132 @@ | |||
import java.util.ArrayList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good listing of imported classes explicitly.
src/main/java/G_one.java
Outdated
System.out.println("OK, I've marked this task as not done yet:"); | ||
System.out.println(" " + task); | ||
} else { | ||
System.out.println("Invalid task number."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good use of error handling
src/main/java/G_one.java
Outdated
G_one g_one = new G_one(); | ||
g_one.start(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps you could add some comments to make your code easier to understand :)
src/main/java/Duke.java
Outdated
System.out.println("Hello! I'm G.one"); | ||
System.out.println("--------------------------------------"); | ||
Scanner scanner = new Scanner(System.in); | ||
boolean flag = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this Boolean variable be named to sound like booleans? eg. isRunning etc
src/main/java/G_one.java
Outdated
} | ||
|
||
System.out.println("Goodbye!"); | ||
scanner.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice detail to close scanner in order to indicate that you're done with its underlying stream!
src/main/java/G_one.java
Outdated
public void start() { | ||
System.out.println("Hello! I'm G.one"); | ||
System.out.println("--------------------------------------"); | ||
|
||
Scanner scanner = new Scanner(System.in); | ||
boolean flag = true; | ||
|
||
while (flag) { | ||
System.out.print("Whats up? "); | ||
String userInput = scanner.nextLine(); | ||
|
||
if (userInput.equalsIgnoreCase("bye")) { | ||
flag = false; | ||
} else if (userInput.equalsIgnoreCase("list")) { | ||
displayTaskList(); | ||
} else { | ||
processUserInput(userInput); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good use of K&R style brackets!
@@ -0,0 +1,13 @@ | |||
public class DeadlineTask extends Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that DeadlineTask
Class name is a noun and written in PascalCase!
src/main/java/G_one.java
Outdated
} | ||
|
||
private void addEventTask(String descriptionAndTime) { | ||
String[] parts = descriptionAndTime.split(" /from "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good that array specifiers are attached to the type not the variable!
src/main/java/G_one.java
Outdated
if (userInput.startsWith("todo")) { | ||
addTodoTask(userInput.substring(5).trim()); | ||
} else if (userInput.startsWith("deadline")) { | ||
addDeadlineTask(userInput.substring(9).trim()); | ||
} else if (userInput.startsWith("event")) { | ||
addEventTask(userInput.substring(6).trim()); | ||
} else if (userInput.startsWith("mark")) { | ||
markTask(userInput.substring(5).trim()); | ||
} else if (userInput.startsWith("unmark")) { | ||
unmarkTask(userInput.substring(7).trim()); | ||
} else if (userInput.equalsIgnoreCase("list")) { | ||
displayTaskList(); | ||
} else { | ||
System.out.println("Invalid command."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good that the if-else statements follows the java conventions!
src/main/java/G_one.java
Outdated
Task task = new DeadlineTask(parts[0], parts[1]); | ||
tasks.add(task); | ||
System.out.println("Alright buddy, Added this:"); | ||
System.out.println(" " + task); | ||
printTaskCount(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SLAP Hard
Avoid having multiple levels of abstraction within a code fragment
You can try write all things in printTaskCount() and rename this function to make more sense
src/main/java/G_one.java
Outdated
if (parts.length != 2) { | ||
System.out.println("Invalid event format."); | ||
return; | ||
} | ||
String[] timeParts = parts[1].split(" /to "); | ||
if (timeParts.length != 2) { | ||
System.out.println("Invalid event format."); | ||
return; | ||
} | ||
Task task = new EventTask(parts[0], timeParts[0], timeParts[1]); | ||
tasks.add(task); | ||
System.out.println("Alright buddy, Added this:"); | ||
System.out.println(" " + task); | ||
printTaskCount(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Deep nesting, Goood
src/main/java/G_one.java
Outdated
private void processUserInput(String userInput) { | ||
if (userInput.startsWith("todo")) { | ||
addTodoTask(userInput.substring(5).trim()); | ||
} else if (userInput.startsWith("deadline")) { | ||
addDeadlineTask(userInput.substring(9).trim()); | ||
} else if (userInput.startsWith("event")) { | ||
addEventTask(userInput.substring(6).trim()); | ||
} else if (userInput.startsWith("mark")) { | ||
markTask(userInput.substring(5).trim()); | ||
} else if (userInput.startsWith("unmark")) { | ||
unmarkTask(userInput.substring(7).trim()); | ||
} else if (userInput.equalsIgnoreCase("list")) { | ||
displayTaskList(); | ||
} else { | ||
System.out.println("Invalid command."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good oop implementation
src/main/java/G_one.java
Outdated
} | ||
|
||
private void unmarkTask(String taskNumberStr) { | ||
int taskNumber = Integer.parseInt(taskNumberStr) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear variable name
src/main/java/G_one.java
Outdated
} | ||
|
||
private void addDeadlineTask(String descriptionAndBy) { | ||
String[] parts = descriptionAndBy.split(" /by "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also clear variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, code is OK. Looks like Lvl 5 has not been done yet, might want to work on that. Naming and adherance to coding standard is good. Mixed usage of abstraction, it can be more consistent. Good job 👍🏽
src/main/java/Duke.java
Outdated
/*String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
+ "|____/ \\__,_|_|\\_\\___|\n";*/ | ||
|
||
//System.out.println("Hello from\n" + logo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid leaving in commented out code. It reduces readbility.
src/main/java/Duke.java
Outdated
//System.out.println("Hello from\n" + logo); | ||
System.out.println("Hello! I'm G.one"); | ||
System.out.println("--------------------------------------"); | ||
Scanner scanner = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider naming this object a bit more intuitively? Someting along the lines of input
?
src/main/java/Duke.java
Outdated
System.out.println("Hello! I'm G.one"); | ||
System.out.println("--------------------------------------"); | ||
Scanner scanner = new Scanner(System.in); | ||
boolean flag = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean variables should be named to sound like booleans. That is, it should be asking a question with a true/false answer.
See: https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#use-name-to-explain
Also see: https://se-education.org/guides/conventions/java/basic.html#naming
src/main/java/Duke.java
Outdated
System.out.println("--------------------------------------"); | ||
Scanner scanner = new Scanner(System.in); | ||
boolean flag = true; | ||
while (flag){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while (flag) {
spacing between ) and { will improve readability
src/main/java/Duke.java
Outdated
while (flag){ | ||
System.out.print("Whats up? "); | ||
String userInput = scanner.nextLine(); | ||
if (userInput.equalsIgnoreCase("bye")){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid magic literals in the code. Maybe consider using a static final variable instead.
src/main/java/G_one.java
Outdated
private void processUserInput(String userInput) { | ||
if (userInput.startsWith("todo")) { | ||
addTodoTask(userInput.substring(5).trim()); | ||
} else if (userInput.startsWith("deadline")) { | ||
addDeadlineTask(userInput.substring(9).trim()); | ||
} else if (userInput.startsWith("event")) { | ||
addEventTask(userInput.substring(6).trim()); | ||
} else if (userInput.startsWith("mark")) { | ||
markTask(userInput.substring(5).trim()); | ||
} else if (userInput.startsWith("unmark")) { | ||
unmarkTask(userInput.substring(7).trim()); | ||
} else if (userInput.equalsIgnoreCase("list")) { | ||
displayTaskList(); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of abstraction. However, can the substring
and trim
portion be separated to a different function dedicated to getting a substring from the input? Mainly because this will reduce complicated expressions and improve the overall readability of this function.
src/main/java/G_one.java
Outdated
} | ||
|
||
private void addDeadlineTask(String descriptionAndBy) { | ||
String[] parts = descriptionAndBy.split(" /by "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parts
is not a very good name. It is unclear as to what it is referring to.
src/main/java/G_one.java
Outdated
printTaskCount(); | ||
} | ||
|
||
private void addDeadlineTask(String descriptionAndBy) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using a different name instead of descriptionAndBy
. It reads a bit confusingly. Maybe deadlineDetails
?
src/main/java/G_one.java
Outdated
public static void main(String[] args) { | ||
G_one g_one = new G_one(); | ||
g_one.start(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, variable names should be in camelCase and classnames in PascalCase.
src/main/java/Task.java
Outdated
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unnecessary space
This reverts commit 7b5aff0.
No description provided.