-
Notifications
You must be signed in to change notification settings - Fork 233
[CS2113-T12-3] TASync #26
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
base: master
Are you sure you want to change the base?
[CS2113-T12-3] TASync #26
Conversation
…th-categories Add JUnit tests and text UI test cases
irving11119
left a comment
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, the code mostly has decent coding quality and there aren't many blatant violations of the coding standards. There are quite a number of methods which are quite long, try to those down into more methods.
Also note the comment on the data, if it is test data, I would advise not to commit this to repo and rather add it to the .gitignore
| System.out.print("[C]"); | ||
| break; | ||
| default: | ||
| break; |
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.
It would be better to catch the default case and throw an error, rather than just breaking
| public Student getStudentByName(String name) { | ||
| Scanner scanner = new Scanner(System.in); | ||
| List<Student> matched = new ArrayList<>(); | ||
|
|
||
| for (Student s : students) { | ||
| if (s.getName().equalsIgnoreCase(name)) { | ||
| matched.add(s); | ||
| } | ||
| } | ||
|
|
||
| if (matched.isEmpty()) { | ||
| System.out.println("No student found with the name: " + name); | ||
| return null; | ||
| } | ||
|
|
||
| if (matched.size() == 1) { | ||
| return matched.get(0); | ||
| } | ||
|
|
||
| boolean allSame = matched.stream().allMatch(s -> s.getName().equalsIgnoreCase(name)); | ||
|
|
||
|
|
||
| System.out.println("Multiple students found with the name '" + name + "':"); | ||
|
|
||
| for (int i = 0; i < matched.size(); i++) { | ||
| Student s = matched.get(i); | ||
| System.out.println((i + 1) + ". " + s.getName() + " (Matric No: " + s.getMatricNumber() + ")"); | ||
| } | ||
|
|
||
| if (allSame) { | ||
| System.out.print("Please enter the matric number to identify the student: "); | ||
| String matric = scanner.nextLine(); | ||
| return getStudentByMatricNumber(matric); | ||
| } else { | ||
| System.out.print("Enter the number (1-" + matched.size() + ") or full name: "); | ||
| String input = scanner.nextLine(); | ||
|
|
||
| try { | ||
| int index = Integer.parseInt(input); | ||
| if (index >= 1 && index <= matched.size()) { | ||
| return matched.get(index - 1); | ||
| } else { | ||
| System.out.println("Invalid index."); | ||
| } | ||
| } catch (NumberFormatException e) { | ||
| for (Student s : matched) { | ||
| if (s.getName().equalsIgnoreCase(input)) { | ||
| return s; | ||
| } | ||
| } | ||
| System.out.println("No match for input name."); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
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 quite long, you could break this down into multiple private methods. Also note ideally your public functions should all have Java Docs.
| /** | ||
| * Prints the schedule for today including todos, deadlines, events, consultations, and tutorial classes. | ||
| */ | ||
| public void displayScheduleForToday() { |
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 quite long, try to break it down into multiple smaller methods. Also do try your best to write Java docs for your public methods
| String tutorialInfo = " - " + "Tutorial Name: " + tutorial.getTutorialName() + | ||
| ", Day of Week: " + tutorial.getDayOfWeek() + | ||
| ", From: " + tutorial.getStartTime() + | ||
| ", To: " + tutorial.getEndTime(); | ||
| System.out.println(tutorialInfo); |
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 have a number of complicated print statements. Consider delegating these functionalities to your UI class.
| ? String.valueOf(DateTimeFormatterUtil.parseDateTime(consultationStart)) | ||
| : "INVALID DATE"; | ||
| String formattedEnd = isEndValid | ||
| ? String.valueOf(DateTimeFormatterUtil.parseDateTime(consultationEnd)) | ||
| : "INVALID DATE"; |
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.
Try to avoid using magic strings in your code, in this case "INVALID DATE" is a hardcoded constant
| try { | ||
| // Validate input string | ||
| if (input == null || input.trim().isEmpty()) { | ||
| throw TASyncException.invalidNewTutorialCommand(); | ||
| } | ||
|
|
||
| // Split the input to get tutorial details | ||
| String[] inputParts = input.split("\\s+"); | ||
| if (inputParts.length != 4) { | ||
| throw TASyncException.invalidNewTutorialCommand(); | ||
| } | ||
|
|
||
| String tutorialName = inputParts[0]; | ||
| String dayOfWeekStr = inputParts[1]; | ||
| String startTimeStr = inputParts[2]; | ||
| String endTimeStr = inputParts[3]; | ||
|
|
||
| // Parse and validate day of the week | ||
| int dayOfWeek; | ||
| try { | ||
| dayOfWeek = Integer.parseInt(dayOfWeekStr); | ||
| if (dayOfWeek < 1 || dayOfWeek > 7) { | ||
| throw TASyncException.invalidDayOfWeek(); | ||
| } | ||
| } catch (NumberFormatException e) { | ||
| throw TASyncException.invalidDayOfWeek(); | ||
| } | ||
|
|
||
| // Parse and validate start and end time | ||
|
|
||
| LocalTime startTime = LocalTime.parse(startTimeStr); | ||
| LocalTime endTime = LocalTime.parse(endTimeStr); | ||
|
|
||
| // Check if the tutorial already exists in the list | ||
| for (TutorialClass existingTutorial : tutorialClassList.getTutorialClasses()) { | ||
| if (existingTutorial.getTutorialName().equals(tutorialName) && | ||
| existingTutorial.getDayOfWeek().getValue() == dayOfWeek && | ||
| existingTutorial.getStartTime().equals(startTime) && | ||
| existingTutorial.getEndTime().equals(endTime)) { | ||
| throw TASyncException.duplicateTutorial(); | ||
| } | ||
| } | ||
|
|
||
| // Create a new TutorialClass object | ||
| StudentList emptyStudentList = new StudentList(); // Empty list for students | ||
| TutorialClass newTutorial = new TutorialClass(); | ||
| newTutorial.setTutorialName(tutorialName); | ||
| newTutorial.setDayOfWeek(DayOfWeek.of(dayOfWeek)); // Convert int to DayOfWeek | ||
| newTutorial.setStartTime(startTime); | ||
| newTutorial.setEndTime(endTime); | ||
| newTutorial.setStudentList(emptyStudentList); // Set an empty student list | ||
|
|
||
| // Add the new tutorial to the tutorial class list | ||
| tutorialClassList.addTutorialClass(newTutorial); | ||
|
|
||
| // Output success message | ||
| System.out.println("New tutorial added: " + newTutorial); | ||
|
|
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 pretty long, you try to break it down more
|
|
||
| public class ViewStudentCommentsCommand implements Command<AttendanceFile> { | ||
| //parts in format of tutname,week,studentname,matricnum | ||
| public void execute(String parts, AttendanceFile attendanceList) { |
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.
Try your best to document public methods with Java Docs
| public static Command createCommand(String commandString) { | ||
| String[] parts = commandString.split("\\s+", 3); // Split into command, type, and rest of input | ||
| if (parts.length < 2) { | ||
| System.out.println("Invalid command format. Please use: /add -[type] [task details]"); | ||
| return null; | ||
| } | ||
|
|
||
| // Remove the leading '/' and convert the command to uppercase | ||
| String command = parts[0].toUpperCase(); | ||
| String taskTypeShortcut = parts[1]; | ||
| String listType = parts[1]; | ||
|
|
||
| if ("HELP".equals(command)) { | ||
| CommandListPrinter.printCommands(); | ||
| return null; | ||
| } | ||
|
|
||
| if ("ADD".equals(command)) { | ||
| TaskType taskType = TaskType.fromShortcut(taskTypeShortcut); | ||
| if (taskType == null) { | ||
| System.out.println("Invalid task type. Use -c (Consultation), -pt (Todo), -pe (Event), -pd (Deadline)"); | ||
| return null; | ||
| } | ||
|
|
||
| switch (taskType) { | ||
| case TODO: | ||
| return new TodoCommand(); | ||
| case EVENT: | ||
| return new EventCommand(); | ||
| case DEADLINE: | ||
| return new DeadlineCommand(); | ||
| case CONSULTATION: | ||
| return new ConsultationCommand(); | ||
| default: | ||
| System.out.println("Unknown task type: " + taskType); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| // Handle TaskList commands | ||
| if (listType.equalsIgnoreCase("-p")) { | ||
| switch (command) { | ||
| case "DELETE": | ||
| return new DeleteTaskCommand(); | ||
| case "MARK": | ||
| return new MarkTaskCommand(); | ||
| case "UNMARK": | ||
| return new UnmarkTaskCommand(); | ||
| case "LIST": | ||
| return new ListTaskCommand(); | ||
| case "FIND": | ||
| return new FindTaskCommand(); | ||
| case "RENAME": | ||
| return new RenameTaskCommand(); | ||
| default: | ||
| System.out.println("Sorry, TASync does not know what \"" + command + "\" means."); | ||
| CommandListPrinter.printCommands(); | ||
| return null; | ||
| } | ||
| } else if (listType.equalsIgnoreCase("-t")) { | ||
| switch (command) { | ||
| case "NEWTUTORIAL": | ||
| return new NewTutorialCommand(); | ||
| case "DELETE": | ||
| return new DeleteTutorialCommand(); | ||
| case "LIST": | ||
| return new ListUpcomingTutorialsCommand(); | ||
| case "NEWSTUDENT": | ||
| return new NewStudentCommand(); | ||
| case "DELETESTUDENT": | ||
| return new DeleteStudentCommand(); | ||
| case "LISTSTUDENTS": | ||
| return new ListTutorialStudentsCommand(); | ||
| case "FIND": | ||
| return new FindStudentCommand(); | ||
| case "CHANGEREMARK": | ||
| return new ChangeRemarkCommand(); | ||
| case "CHECKREMARK": | ||
| return new CheckRemarkCommand(); | ||
| default: | ||
| System.out.println("Sorry, TASync does not know what \"" + command + "\" means."); | ||
| CommandListPrinter.printCommands(); | ||
| return null; | ||
| } | ||
| } else if (listType.equalsIgnoreCase("-a")) { | ||
| switch (command) { | ||
| case "MARK": | ||
| return new MarkStudentAttendanceCommand(); | ||
| case "UNMARK": | ||
| return new UnmarkStudentAttendanceCommand(); | ||
| case "COMMENT": | ||
| return new CommentOnStudentCommand(); | ||
| case "VIEWCOMMENT": | ||
| return new ViewStudentCommentsCommand(); | ||
| case "DELETECOMMENT": | ||
| return new DeleteStudentComment(); | ||
| case "LIST": | ||
| return new ShowAttendanceListCommand(); | ||
| default: | ||
| System.out.println("Sorry, TASync does not know what \"" + command + "\" means."); | ||
| CommandListPrinter.printCommands(); | ||
| return null; | ||
| } | ||
| } else if (listType.equalsIgnoreCase("-at")) { | ||
| if (command.equals("CREATE")) { | ||
| return new CreateNewAttendanceList(); | ||
| } | ||
| System.out.println("Sorry, TASync does not know what \"" + command + "\" means."); | ||
| CommandListPrinter.printCommands(); | ||
| return null; | ||
| } | ||
|
|
||
| if (command.equals("BYE")) { | ||
| return new ByeCommand(); | ||
| } | ||
|
|
||
| return null; | ||
| } |
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 pretty long, try to break it down into smaller parts
data/AllTutorials.csv
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.
I think it would be advisable not to commit test data for your program. Your data should be generated fresh when running the jar file and then subsequently saved. Assuming this is all test data generated, you should add it to the .gitgnore file so its not committed.
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 add this to the .gitignore as well
Dev guide for AttendanceListCommands
- Abstract setUp code for each test in order to reduce code duplication - Create ChangeRemarkCommandTest, CheckRemarkCommandTest, DeleteStudentCommandTest,FindStudentCommandTest and NewStudentCommandTest for easy testing of methods
Add tests for more studentcommands
Add JUnit test for task commands
jessican1212
left a comment
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, I think the written portion of your Developer Guide is very strong and I had a difficult time finding errors to critique. The professor did mention that there needs to be more diagrams. I think that is the main issue.
| - Removes the task from the `TaskList`. | ||
| - Displays a confirmation message upon successful removal. | ||
| - If the task is not found, an error message is shown. | ||
|
|
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 believe the DG should include diagrams (UML diagrams, class diagrams, sequence diagrams, etc.)
|
|
||
| The class implements the following main operation: | ||
|
|
||
| - `NewStudentCommand#execute()` |
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.
Is this a typo? Should it be NewStudentcommand.execute()?
docs/DeveloperGuide.md
Outdated
| - The hashMap containing the attendance status of the students will be printed. | ||
| - A different message will be printed if this hashMap is empty(signalling that the attendanceList has no students in it). | ||
| - An error message will be printed when an Exception is handled. | ||
| - |
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 bullet point should probably be deleted if it's empty!
docs/DeveloperGuide.md
Outdated
|
|
||
| #### Implementation details | ||
| This class implements the Command<AttendanceList> interface.Its function is to take in the input from the user and extract the tutorial and week the user has given | ||
| and print the names and attendance status of the students in that tutorial,if it exists. |
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.
Ensure to fix small typos such as forgotten spaces after periods and commas for professionalism.
Update developer guide for task commands
- Amend the formatting of the User Guide - Add student command details for users - Add important notes for users to take note of during command execution
- Amend the formatting of the User Guide - Add student command details for users - Add important notes for users to take note of during command execution
Add Junit tests for marks and marksList.
Add DG documentation for marks commands.
jing-yen
left a comment
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 effort for your DG. Can be improved by explaning the architecture and supplementing it with UML diagrams
docs/DeveloperGuide.md
Outdated
| {list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | ||
|
|
||
| ## Design & implementation | ||
| ## **Design & implementation** |
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.
It would be a good idea to describe the architecture of TASync project, before explaining the specifics (an architecture diagram would be very welcome!)
|
|
||
| - `attendancelistcommands` - Handles commands related to attendance lists. | ||
| - `commandhandler` - Manages the processing and execution of various commands | ||
| - `student commands` - Implements commands specific to student-related operations |
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.
Ensure correct naming of the sub-packages. For instance, the space should be omitted from student commands sub-package.
docs/DeveloperGuide.md
Outdated
| - Parses the input to extract the tutorial class code and matriculation number. | ||
| - Validates the input to ensure it follows the expected format and does not contain any errors. | ||
| - Retrieves the specified tutorial class based on the provided class code. | ||
| - Checks if the tutorial class exists. If it does not, an error message is shown. | ||
| - Retrieves the student list from the tutorial class. | ||
| - Checks if the student with the given matriculation number exists in the class. | ||
| - If the student exists, retrieves their remark. | ||
| - If the student has a remark, displays it. Otherwise, indicates that no remarks were found. | ||
| - If the student does not exist, an error message is shown indicating that no such student was found. | ||
| - Handles any exceptions by displaying relevant error messages if validation fails or the tutorial class is not found. |
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.
For step-by-step operations, consider using numerical bullets (with possible sub-lists for conditional cases) instead of round bullets.
docs/DeveloperGuide.md
Outdated
| and print the names and attendance status of the students in that tutorial,if it exists. | ||
|
|
||
| #### operations | ||
| - `ShowAttendanceListCommand.execute()` |
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 operation code should be unindented (or the following description indented more) for clarity.
docs/DeveloperGuide.md
Outdated
| - `ShowAttendanceListCommand.execute()` | ||
| - extracts tutorial name and week number from the given input. | ||
| - Exception thrown if no input or not enough input given. | ||
| - The AttendancFile is searched to find the relevant AttendanceList |
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.
AttendancFile should be AttendanceFile
update default value of Student Class
- Amend the formatting of the User Guide - Add tutorial command details for users - Add important notes for users to take note of during command execution
Update user guide
checkstyle fixes
Add class diagram for marks.
Fix bug in class diagram
- Make them less detailed but easier to comprehend to readers - Change DG formatting
Update class diagrams
Upload png files for class diagrams
Edit command name for UG
Update NewTutorialCommandSequenceDiagram.puml
update sequence diagram to remove numbering
Standardise test case format in appendix E
Update User Guide
Stop tracking data folder
Update PPP to meet page limits
Provide convenient management of tutor’s administrative tasks like attendance, remarks and planning, optimized for CLI.