[CS2113-W12-2] FindOurSEP #8
Conversation
There was a problem hiding this comment.
Some suggestions for the team to consider. Please standardise how empty lines are used in your codebase. Try not to use excessive empty lines as they are meant to group different logics, instead of simply in between every line/block. Overall good work, keep it up.
| * @return The created Student object if all validations pass. | ||
| * @throws SEPException If any validation errors occur, a SEPException containing all error messages is thrown. | ||
| */ | ||
| public Student makeStudent(String input) throws SEPException { |
There was a problem hiding this comment.
createStudent might sound better haha?
src/main/java/parser/Parser.java
Outdated
| public boolean parseInput(String input) { | ||
| String[] parts = input.split(" "); | ||
| switch (parts[0].toLowerCase()) { | ||
| case "add": |
There was a problem hiding this comment.
You could consider extracting the commands into constant as well for easy usage in other places or change later on.
| Future<StudentList> allocationTask = executor.submit(() -> { | ||
| return allocator.allocate(); | ||
| }); | ||
|
|
||
| // Submit the loading message task | ||
| Future<?> loadingTask = executor.submit(() -> { | ||
| while (!allocationTask.isDone()) { | ||
| this.ui.printAllocatingMessage(); | ||
| } | ||
| }); | ||
|
|
||
| // Check periodically if the allocation is done | ||
| while (!allocationTask.isDone()) { | ||
| Thread.sleep(100); // Small delay to avoid busy-waiting | ||
| } | ||
|
|
||
| // Interrupt the loading task once allocation is done | ||
| loadingTask.cancel(true); | ||
|
|
||
| // Retrieve the result of the allocation task | ||
| this.studentList.setStudentList(allocationTask.get()); | ||
|
|
||
| // Shut down the executor service | ||
| executor.shutdown(); | ||
|
|
There was a problem hiding this comment.
You could consider only leave spaces between different groups of logics, instead of between every line or block.
src/main/java/ui/UI.java
Outdated
| Thread.sleep(500); // 500 milliseconds | ||
| } | ||
| } | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); // Restore the interrupted status | ||
| } finally { | ||
| System.out.println("\r" + Messages.ALLOCATE_COMPLETE); | ||
| System.out.println(HORIZONTAL_LINE); | ||
| } | ||
| } |
There was a problem hiding this comment.
You could consider converting some numbers and strings into named constant to give it a meaning. It will be easier for future usage or change as well.
| throw SEPEmptyException.rejectEmptyStudentList(); | ||
| } | ||
|
|
||
| AsciiTable at = new AsciiTable(); |
There was a problem hiding this comment.
You could consider give the variable a more meaningful name by giving a bit more description.
| * Manages a list of students and provides methods for adding, deleting, sorting, | ||
| * printing and validating student data such as student ID, GPA, and preferences. | ||
| */ | ||
| public class StudentList { |
There was a problem hiding this comment.
This class is pretty long, you might consider extracting some methods into another helper class and invoke the relevant methods if needed, e.g. splitInput, validateStudentId etc.
docs/DeveloperGuide.md
Outdated
| #### Allocate Command | ||
|
|
||
| #### Revert Command | ||
|
|
There was a problem hiding this comment.
Diagram image for RevertSequence and GenerateSequence is currently broken
| @@ -1,22 +1,241 @@ | |||
| # Developer Guide | |||
|
|
|||
| ## Table of Contents | |||
There was a problem hiding this comment.
Overall there are no major errors with the UML diagrams presented. Good job
|
|
||
| Here is a class diagram highlighting the fundamental structure of the `Allocator` class. | ||
|
|
||
|  |
There was a problem hiding this comment.
The Solid Arrow for the class diagrams is incorrect. It should be a solid Triangle. You can refer to the Parser class diagram for the correct implementation.
|
|
||
| ``Allocator`` mainly participates in the execution of ``allocate`` command. The sequence diagram below showcases the program workflow when a user inputs the command ``allocate`` (assume before that several students have been added into the student list). | ||
|
|
||
|  |
There was a problem hiding this comment.
You can consider keeping the styling for all the names of the entities constant (Bolding of :Allocator entity).
|
|
||
| Here is a class diagram highlighting the fundamental structure of the `Parser` class. | ||
|
|
||
|  |
There was a problem hiding this comment.
You could consider segmenting out the {abstract} Command class to improve its visibility, it currently looks like another command class.
Sequence Diagrams for Criteria Command and Delete Command
Add Sequence diagram for find and filter commands
Add Sequence Diagram for Stats, Allocate, and ViewQuota Commands
Change some small bugs in the DG
docs/DeveloperGuide.md
Outdated
| The `UI` class methods typically return `void`, but will print responses directly to the console or handle user input, | ||
| streamlining interactions and allowing the backend to focus solely on data processing. |
There was a problem hiding this comment.
Maybe its better to include when does it not return void
docs/DeveloperGuide.md
Outdated
|
|
||
| The sequence diagram below demonstrates the interactions within the `Parser` component when a user inputs the command: `add id/A1234567I gpa/5.0 p/{13,61,43}`. | ||
|
|
||
|  |
docs/DeveloperGuide.md
Outdated
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
| Delete Command removes an exisiting Student in the StudentList. |
There was a problem hiding this comment.
Maybe can explain in greater detail, particularly what is the return string for.
docs/DeveloperGuide.md
Outdated
|
|
||
| #### Criteria Command | ||
|
|
||
| Criteria Command sets a minimum GPA every student must acheieve before they can be allocated to a university. |
There was a problem hiding this comment.
May be irrelevant to DG, but the method name is inconsistent ('gpa' in setMinimumGPA and validateGpa).
docs/DeveloperGuide.md
Outdated
|
|
||
| Criteria Command sets a minimum GPA every student must acheieve before they can be allocated to a university. | ||
|
|
||
|  |
There was a problem hiding this comment.
Similarly, its nice to explain how the returned float is used.
|
|
||
| The `StatCommand` class implements the `stats` command, which provides GPA-related statistics (average GPA or minimum GPA) for students associated with a specified university. The command syntax is `stats <stat_type> <UNI_INDEX>`, where `<stat_type>` can be `-avggpa` for average GPA or `-mingpa` for minimum GPA. | ||
|
|
||
|  |
There was a problem hiding this comment.
I think its better to standardize all the sequence diagrams for your commands. Either make the activation bar starts form the Command class directly or use dotted line first.
|
|
||
| The `ViewQuotaCommand` class handles the `viewQuota` command to display information about a university’s remaining quota (available spots) based on a specified university index. | ||
|
|
||
|  |
There was a problem hiding this comment.
I think its better to leave the return variable name, spotsLeft out, since all the diagrams earlier do not have it.
docs/DeveloperGuide.md
Outdated
|
|
||
| The `AllocateCommand` class manages the allocation process of students using the `Allocator` class. This command sets up an allocation process for students in the specified `StudentList` and informs the user that allocation is underway. | ||
|
|
||
| You could refer to [Allocator](#allocator) section to check the detailed workflow of ``AllocateCommand``. |
There was a problem hiding this comment.
printResponse return arrow is not pointing properly.
|
|
||
| #### Generate Command | ||
|
|
||
|  |
There was a problem hiding this comment.
the return arrow of Ui is not pointing properly
docs/UML_Diagrams/UIClass.drawio.svg
Outdated
There was a problem hiding this comment.
Based on your source code (caa 31/10/24 1531H):
public class UI {
private static final int LINE_LENGTH = 80;
public static final String HORIZONTAL_LINE = "-".repeat(LINE_LENGTH);
private final Scanner scanner;
...
}
should the LINE_LENGTH and HORIZONTAL_LINE attributes in this diagram be underlined instead (to show that they are static)?
There was a problem hiding this comment.
Additionally, would it be better if the default values of your attributes are indicated in the class diagram (e.g. - LINE_LENGTH: int = 80
There was a problem hiding this comment.
Should the self invocations done by :StudentList have a return arrow too?
There was a problem hiding this comment.
Is there a better to way to express this loop:
given that your source code in Parser.java contains this code (caa 31/10/24 1545H):
public void deleteStudent(String input) throws SEPException{
String[] parts = input.split("delete", 2);
String studentId = organiseId(parts[1]);
if (!studentId.matches(ID_REGEX)) {
throw SEPFormatException.rejectIdFormat();
}
boolean isRemoved = students.removeIf(student -> student.getId().equals(studentId));
if (!isRemoved) {
throw SEPEmptyException.rejectStudentNotFound();
}
}
There is no mention of the :String that is returned from StudentList::getId.
There was a problem hiding this comment.
Should the operation and return arrows in this diagram leave a small gap in the activation bar?
I understand that the respective methods are called/the methods are returned immediately, but most of the examples in the textbook show a small gap:
Update DeveloperGuide.md according to comments
Update PPP
Small fixes
Change Student class diagram link name
last fix to deletecommand diagram
Update Ug regarding preferences
Update ppp
Update UG
Adjust page break
Adjust UG
FindOurSEP is a tool designed for NUS administrative staff managing Student Exchange Program (SEP) allocations for Computer Engineering students. It streamlines the process of matching students with suitable universities based on academic profiles, preferences, and other criteria. This tool aims to enhance efficiency and accuracy in the complex task of SEP placement management.