-
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
[LIU SIYI] iP #184
base: master
Are you sure you want to change the base?
[LIU SIYI] iP #184
Conversation
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 it looks good. Although there were some minor coding standard violations.
src/main/java/UserDetails.java
Outdated
switch (input) { | ||
case "1": | ||
gender = "Male"; | ||
break; | ||
case "2": | ||
gender = "Female"; | ||
break; | ||
case "3": | ||
gender = "Other"; | ||
break; | ||
default: | ||
System.out.println("Invalid option selected. Please enter 1, 2, or 3."); | ||
continue; | ||
} | ||
break; // Exit the loop once a valid input is received. |
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.
There should not be any indentation for case clauses to follow the coding standards.
src/main/java/Main.java
Outdated
String message = String.format( | ||
"Verification passed.\nOptions enabled.\n%s Born on %s\n%s\nID A.D.0013\n" + | ||
"Rank 'S'\nListed in the Kassel Academy roster.\n" + | ||
"Database access granted\nAccount activated\nCourse schedule generated\n" + | ||
"I am Erii, the secretary of Kassel Academy, pleased to serve you.", | ||
name, birthday, gender); |
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.
I like the line wrapping. It makes it more readable.
src/main/java/TaskManager.java
Outdated
@@ -0,0 +1,39 @@ | |||
// TaskManager.java | |||
public class TaskManager { | |||
private static final String[] tasks = new String[100]; |
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.
Can consider setting 100 to a constant variable.
src/main/java/TaskManager.java
Outdated
public static void addTask(String task) { | ||
if (taskCount < tasks.length) { | ||
tasks[taskCount++] = task; | ||
System.out.println("____________________________________________________________"); |
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.
Can consider changing divider to a constant.
src/main/java/UserDetails.java
Outdated
* | ||
* @return the user's full name | ||
*/ | ||
public static String getName() { |
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.
Naming of variables are good as it follows the naming conventions of using camelCase.
src/main/java/TaskManager.java
Outdated
public static boolean processCommand(String command) { | ||
if ("list".equalsIgnoreCase(command)) { | ||
listTasks(); | ||
return true; | ||
} else if ("bye".equalsIgnoreCase(command)) { | ||
System.out.println("____________________________________________________________"); | ||
System.out.println(" Bye. Hope to see you again soon!"); | ||
System.out.println("____________________________________________________________"); | ||
return false; | ||
} else { | ||
addTask(command); | ||
return 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.
Good indentation
src/main/java/UserDetails.java
Outdated
switch (input) { | ||
case "1": | ||
gender = "Male"; | ||
break; | ||
case "2": | ||
gender = "Female"; | ||
break; | ||
case "3": | ||
gender = "Other"; | ||
break; | ||
default: | ||
System.out.println("Invalid option selected. Please enter 1, 2, or 3."); | ||
continue; |
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.
case and switch should be aligned, invalid indentation
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 code quality
src/main/java/Erii.java
Outdated
@@ -0,0 +1,27 @@ | |||
public class Erii { | |||
public static void main(String[] args) { | |||
String art = |
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 move this to another file?
src/main/java/UserDetails.java
Outdated
@@ -0,0 +1,88 @@ | |||
import java.util.Scanner; | |||
import java.util.regex.Matcher; |
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.
i like that you use regex
src/main/java/UserDetails.java
Outdated
System.out.println("Please enter your full name (First Name Last Name): "); | ||
String name; | ||
while (true) { | ||
name = scanner.nextLine().trim(); |
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 using trim to remove unnecessary characters
src/main/java/TaskManager.java
Outdated
} | ||
|
||
public static boolean processCommand(String command) { | ||
if ("list".equalsIgnoreCase(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 you have a method to ignore case
src/main/java/Erii.java
Outdated
@@ -0,0 +1,27 @@ | |||
public class Erii { | |||
public static void main(String[] args) { | |||
String art = |
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.
Make variable names for constant all uppercase.
src/main/java/UserDetails.java
Outdated
import java.util.Scanner; | ||
import java.util.regex.Matcher; |
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.
Import classes listed explicitly, good
Organized classes into dedicated packages for better modularity and maintainability. Created com.erii as the base package and subdivided functionalities into core, ui, and user packages respectively. Addresses issue nus-cs2113-AY2324S2#123 for project structure optimization.
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 job, you can improve upon abstraction of the code. Else, well done. 👍🏽
src/main/java/com/erii/Main.java
Outdated
// Prompt the user to enter their details | ||
String name = UserDetails.getName(); | ||
String birthday = UserDetails.getBirthday(); | ||
String gender = UserDetails.getGenderSelection(); | ||
|
||
|
||
// Print the user details with a welcome message | ||
String message = String.format( | ||
"Verification passed.\nOptions enabled.\n%s Born on %s\n%s\nID A.D.0013\n" + | ||
"Rank 'S'\nListed in the Kassel Academy roster.\n" + | ||
"Database access granted\nAccount activated\nCourse schedule generated\n" + | ||
"I am Erii, the secretary of Kassel Academy, pleased to serve you.", | ||
name, birthday, gender); | ||
|
||
System.out.println(message); | ||
|
||
//Call control panel to start the task manager | ||
ControlPanel.main(args); |
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.
These comments are not required as it is quite clear from the code already.
|
||
public class Erii { | ||
public static void main(String[] args) { | ||
String art = |
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 storing this art
as a static final variable
|
||
public class TaskManager { | ||
|
||
public enum Priority { |
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 enum
SS, S, A, B, C, D | ||
} | ||
|
||
public abstract class 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.
avoid declaring multiple classes within a same file, try an separate them into differen files (to improve readability)
System.out.println("Got it. I've added this task:"); | ||
System.out.println(" " + task); | ||
System.out.println("Now you have " + tasks.size() + " tasks in the list."); | ||
System.out.println("____________________________________________________________"); |
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.
you can store is line as a static final variable instead
private static void addDeadlineTask(String input, TaskManager taskManager) { | ||
String[] parts = input.split(" ?/by | ?/ ?"); | ||
if(parts.length < 3 || parts[1].isEmpty() || parts[2].isEmpty()){ | ||
System.out.println("Incorrect format. Please ensure the task description is followed by '/by' and a deadline date, and then a priority value (e.g., 'submit report /by 2021-09-30 /SS')."); |
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.
similarly here, split into 2 lines to avoid exceeding the line character limit.
} | ||
} | ||
|
||
private static String getAddress() { |
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 name of this function is a little misleading perhaps? I understand it probably refers to how to 'address' someone, but maybe use a word like salutation
or userTitle
instead?
private static final Scanner scanner = new Scanner(System.in); | ||
private static final Pattern datePattern = Pattern.compile("^\\d{2}/\\d{2}/\\d{4}$"); |
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.
Constant names must be all uppercase using underscore to separate words (aka SCREAMING_SNAKE_CASE).
https://se-education.org/guides/conventions/java/basic.html#naming
private static String UserName = ""; | ||
private static String UserBirthday = ""; | ||
private static String UserGender = ""; |
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.
Variable names must be in camelCase.
https://se-education.org/guides/conventions/java/basic.html#naming
|
||
public class ControlPanel { | ||
|
||
public static void main(String[] args) { |
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.
this method is really long (>30 LOC) shorten it by abstracting out parts of code into functions
No description provided.