[CS2113-W13-4] TutorLink #13

RCPilot1604 · 2024-09-19T05:28:33Z

NUS professors juggle admin tasks like tracking attendance and managing assignments, which detracts from their time for lesson preparation. Our app simplifies these processes by automating routine tasks and presenting concise, essential information, freeing up valuable time and enhancing their focus on teaching.

Mahesh1772

Great job overall! The code is solid, and the functionality is well-implemented. I only had a few minor recommendations, mainly around Javadocs, using constants and naming, but nothing that detracts from the quality of the work. Thanks for the effort and attention to detail here!

Mahesh1772 · 2024-10-27T02:30:19Z

src/test/java/tutorlink/command/DeleteGradeCommandTest.java

+    private GradeList addGradeForTest(Parser parser, AppState appState, String componentName) {
+        //Create student
+        String line = "add_student i/A1234567X n/John Doe";
+        Command addStudentCommand = new AddStudentCommand();
+        String[] argumentPrefixes = addStudentCommand.getArgumentPrefixes();
+        HashMap<String, String> arguments = parser.getArguments(argumentPrefixes, line);
+        CommandResult result = addStudentCommand.execute(appState, arguments);
+        assertNotNull(result);
+        assertEquals("Student John Doe (A1234567X) added successfully!", result.toString());
+        assertEquals(appState.students.getStudentArrayList().size(), 1);
+
+        //Create component
+        String examName = componentName;
+        double examMaxScore = 100.0;
+        double examWeight = 50.0;
+        Exam exam = new Exam(examName,examMaxScore, examWeight);
+
+        appState.components.addComponent(exam);
+
+        //Add grade
+        HashMap<String, String> gradeArguments = new HashMap<>();
+
+        String matricNumber = "A1234567X";
+        String componentDescription = componentName;
+        String scoreNumber = "75.0";
+        gradeArguments.put("i/",matricNumber);
+        gradeArguments.put("c/", componentDescription);
+        gradeArguments.put("s/", scoreNumber);
+
+        Command addGradeCommand = new AddGradeCommand();
+        CommandResult gradeResult = addGradeCommand.execute(appState, gradeArguments);
+
+        //Test grade added
+        assertNotNull(gradeResult);
+        assertEquals(String.format(successMessage, scoreNumber, componentDescription, matricNumber),
+                gradeResult.toString());
+
+        return appState.grades;
+    }


addGradeForTest() is too long (>30 LOC) and doing multiple things, violating the "Avoid Long Methods" and the "Single Responsibility" principle. Consider breaking it down into smaller helper methods.

Mahesh1772 · 2024-10-27T02:31:07Z

src/test/java/tutorlink/command/DeleteGradeCommandTest.java

+        double examMaxScore = 100.0;
+        double examWeight = 50.0;


Consider converting magic numbers and strings to constants, to increase maintainability.

Mahesh1772 · 2024-10-27T02:31:27Z

src/test/java/tutorlink/command/DeleteGradeCommandTest.java

+
+    private GradeList addGradeForTest(Parser parser, AppState appState, String componentName) {
+        //Create student
+        String line = "add_student i/A1234567X n/John Doe";


Variable names could be more descriptive

Mahesh1772 · 2024-10-27T02:32:27Z

src/test/java/tutorlink/command/DeleteGradeCommandTest.java

+        HashMap<String, String> arguments = parser.getArguments(argumentPrefixes, line);
+        CommandResult result = addStudentCommand.execute(appState, arguments);
+        assertNotNull(result);
+        assertEquals("Student John Doe (A1234567X) added successfully!", result.toString());


The success string could be added to the UI class

Mahesh1772 · 2024-10-27T02:34:15Z

src/test/java/tutorlink/command/FindStudentCommandTest.java

+    }
+
+    @Test
+    void execute_matric_one() {


Test method names don't follow the recommended format featureUnderTest_testScenario_expectedBehavior(). Consider renaming.

Mahesh1772 · 2024-10-27T02:43:54Z

src/test/java/tutorlink/lists/ComponentListTest.java

+        comp1 = new Assignment("Homework 1", 30, 0.1);
+        comp2 = new Assignment("Homework 2", 30, 0.1);
+        comp3 = new Exam("Finals", 100, 0.5);


Consider using more descriptive variable names

Mahesh1772 · 2024-10-27T02:44:38Z

src/test/java/tutorlink/lists/ComponentListTest.java

+    }
+
+    @Test
+    void toString_test() {


Ensure to follow the format featureUnderTest_testScenario_expectedBehavior() like you have with others

Mahesh1772 · 2024-10-27T02:45:21Z

src/test/java/tutorlink/lists/ComponentListTest.java

+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class ComponentListTest {


Consider adding Javadocs

Mahesh1772 · 2024-10-27T02:49:33Z

src/main/java/tutorlink/lists/GradeList.java

+    public double calculateStudentGPA(String matricNumber, ComponentList componentList)
+            throws IncompleteGradesException {
+        ArrayList<Grade> studentGrades = gradeArrayList
+                .stream()
+                .filter(grade -> grade.getStudent().getMatricNumber().equals(matricNumber.toUpperCase()))
+                .collect(Collectors.toCollection(ArrayList::new));
+
+        ArrayList<Component> allComponents = componentList.findAllComponents();
+
+        if (studentGrades.isEmpty()) {
+            throw new StudentNotFoundException("No grades found for student: " + matricNumber);
+        }
+
+        boolean hasAllComponents = allComponents.stream()
+                .allMatch(component -> studentGrades.stream()
+                        .anyMatch(grade -> grade.getComponent().equals(component)));
+
+        if (!hasAllComponents) {
+            throw new IncompleteGradesException(
+                    "Cannot calculate GPA: Student " + matricNumber + " is missing grades for some components");
+        }
+
+        return studentGrades
+                .stream()
+                .mapToDouble(grade -> grade.getScore() * grade.getComponent().getWeight())
+                .sum();
+    }


Method is doing too many things (SLAP violation) - consider breaking into smaller methods. Magic number 0.07 in tax calculation could be a constant

Mahesh1772 · 2024-10-27T02:50:19Z

src/main/java/tutorlink/lists/GradeList.java

+    public boolean deleteGrade(String matricNumber, String componentDescription) {
+        matricNumber = matricNumber.toUpperCase();
+        componentDescription = componentDescription.toLowerCase();
+        for (Grade grade : gradeArrayList) {
+            if (grade.getStudent().getMatricNumber().equals(matricNumber)
+                    && grade.getComponent().getName().equals(componentDescription)) {
+                return gradeArrayList.remove(grade);
+            }
+        }
+        return false;
+    }


Consider using early return pattern to reduce nesting

yakultbottle

Overall looks good, only minor questions

yakultbottle · 2024-10-30T06:12:39Z

docs/DeveloperGuide.md

@@ -2,37 +2,189 @@

 ## Acknowledgements

-{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
+{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the
+original source as well}

 ## Design & implementation


Should this header Design & implementation be removed, since there is a separate Implementation header below?

yakultbottle · 2024-10-30T06:22:02Z

docs/DeveloperGuide.md

+The following sequence diagrams depict the exact steps involved in the `AddStudentCommand`:
+
+![AddStudentCommand.png](diagrams/AddStudentCommand.png)
+


Should there be a dotted line indicating the main program calling this function?

yakultbottle · 2024-10-30T06:27:26Z

docs/DeveloperGuide.md

+All commands follow the sequence as described in the diagram below: 
+
+![ArchitectureSequenceGrouped.png](diagrams%2FArchitectureSequenceGrouped.png)
+


Could this constructor be omitted from this diagram, since it is not necessary to demonstrate command flow sequence?

yakultbottle · 2024-10-30T06:34:02Z

docs/DeveloperGuide.md

+
+![GradeStorage.png](diagrams/GradeStorage.png)
+


Could all the instantiations be moved into a separate diagram?

wongwh2002

other than the comments, lgtm!

wongwh2002 · 2024-10-30T06:19:10Z

docs/DeveloperGuide.md


-{Describe the value proposition: what problem does it solve?}
+- At app launch, TutorLink initializes components (<code>Parser</code>, <code>Ui</code>, <code>Storage</code>, <code>
+  AppState</code>).


formatting issue here

wongwh2002 · 2024-10-30T06:20:48Z

docs/DeveloperGuide.md


-## Product scope
-### Target user profile
+![Architecture.png](diagrams/Architecture.png)


confusing diagram, do not understand what the arrows and lines are trying to say, maybe add a legend of what each line

wongwh2002 · 2024-10-30T06:32:25Z

docs/DeveloperGuide.md

+
+All commands follow the sequence as described in the diagram below: 
+
+![ArchitectureSequenceGrouped.png](diagrams%2FArchitectureSequenceGrouped.png)


should the displayException have a return to standardise?

wongwh2002 · 2024-10-30T06:36:20Z

docs/DeveloperGuide.md

+All commands follow the sequence as described in the diagram below: 
+
+![ArchitectureSequenceGrouped.png](diagrams%2FArchitectureSequenceGrouped.png)
+


should you use alt for exception? maybe use alt [success] and [failure]?
would opt [success] be better

wongwh2002 · 2024-10-30T06:37:45Z

docs/DeveloperGuide.md

+
+All commands follow the sequence as described in the diagram below: 
+
+![ArchitectureSequenceGrouped.png](diagrams%2FArchitectureSequenceGrouped.png)


should you concise the alt [success] sequence? the (new StudentComponent, ComponentStorage, GradeStorage) and the rest is a bit messy/confusing

wongwh2002 · 2024-10-30T06:41:11Z

docs/DeveloperGuide.md

+
+Likewise, the sequence diagram for `DeleteStudentCommand` is as follows: 
+
+![DeleteStudentCommand.png](diagrams%2FDeleteStudentCommand.png)


should you have a lifeline for where "main"/ or something calls DeleteStudentCommand for better readability

wongwh2002 · 2024-10-30T06:46:30Z

docs/DeveloperGuide.md

+
+The sequence diagram of the DeleteGradeCommand is shown below.
+
+![AddGradeCommand.png](diagrams%2FAddGradeCommand.png)


should AddStudentCommand have a X at the end of the lifeline of an object to show its deletion

wongwh2002 · 2024-10-30T06:51:15Z

docs/DeveloperGuide.md

+1. TutorLink constructs a new `GradeStorage`.
+2. TutorLink calls `loadGradeList`.
+3. `GradeStorage` creates a new `ArrayList` of `Grade`s.
+4. `GradeStorage` creates a new `Scanner` with the path to the file.
+5. While there are next lines in the data file:
+   - `Scanner` returns the current file line as a String and moves to the next file line.
+   - `GradeStorage` calls its `getGradeFromFileLine` method with the file line.
+   - If the file line references a valid `Component` and a valid `Student`, a `Grade` is returned and added to the `ArrayList`.
+   - If not (e.g. if data file was corrupted), the file line is simply ignored, and the loop continues to the next iteration.
+6. The `ArrayList` of `Grade`s is returned to TutorLink.


should this be more concise instead of going through your entire program/algorithm

wongwh2002 · 2024-10-30T06:51:32Z

docs/DeveloperGuide.md

@@ -2,37 +2,189 @@

 ## Acknowledgements

-{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
+{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the


should you include a class diagram

wongwh2002 · 2024-10-30T06:51:58Z

docs/DeveloperGuide.md

@@ -2,37 +2,189 @@



lgtm in general!

sarahchow03

Overall, the diagrams are well-documented and rigorous! I liked how the classes were colour-coded and standard across all diagrams, and that there was efficient use of reference frames in your team's DG. Good job!

sarahchow03 · 2024-10-31T06:52:15Z

docs/diagrams/ArchitectureSequenceGrouped.png

Should there be some standardization for the function calls in this diagram, specifically for displayResult to be displayResult()?

sarahchow03 · 2024-10-31T06:52:45Z

docs/diagrams/GradeStorage.png

Should the lifeline for Scanner terminate after X, as it indicates the end of the lifeline?

Minor nit: In the alt frame of this diagram, one or two words to indicate what was successful will be good!

sarahchow03 · 2024-10-31T06:58:57Z

docs/diagrams/DeleteStudentCommand.png

Although including activation bars are not compulsory, with reference to the sequence diagram for AddGradeCommand and DeleteGradeCommand, should the activation bar for :DeleteStudentCommand be included to ensure uniformity across your team's diagrams?

sarahchow03 · 2024-10-31T06:59:48Z

docs/diagrams/FindStudentCommand.png

Similar to a previous comment, the activation bar for :FindStudentCommand could be included to ensure uniformity across your team's diagrams!

In the alt frame of this diagram, the second condition has not been labelled. This could be fixed by adding a [else].

sarahchow03 · 2024-10-31T07:00:13Z

docs/diagrams/AddComponentCommand.png

Similar to a previous comment, should the activation bar for :AddComponentCommand be included to ensure uniformity across your team's diagrams?

…veloperGuide

Complete Draft of Developer Guide, except for storage data in appendix E

Clean up DG

Add calcGPA command

…an-7-Nov * 'master' of github.com:AY2425S1-CS2113-W13-4/tp: Modify DeveloperGuide.md Re-export png files Modify ArchitectureSequenceGrouped.puml Modify .puml files Fix ui test again Fix ui test Fix codestyle Fix tests Handle totalWeighting = 0 case Fix updating GPA mechanic Add command # Conflicts: # docs/diagrams/GradeStorage.puml

…factor

Branch UG update

Update UserGuide.md

Update AboutUs.md

Fix ppp link

Branch more javadoc

Update yeekian PPP

Updatedppp

Bugfix for incorrect error message

Add author tags for GradeStorage

Branch fix reposense

Mahesh1772 reviewed Oct 27, 2024

View reviewed changes

yakultbottle approved these changes Oct 30, 2024

View reviewed changes

wongwh2002 reviewed Oct 30, 2024

View reviewed changes

sarahchow03 reviewed Oct 31, 2024

View reviewed changes

yeekian and others added 9 commits November 6, 2024 22:24

Merge branch 'master' of https://github.com/yeekian/tp into branch-De…

54b0934

…veloperGuide

Merge pull request #129 from yeekian/branch-DeveloperGuide

06148b4

Complete Draft of Developer Guide, except for storage data in appendix E

Merge branch 'AY2425S1-CS2113-W13-4:master' into calcGPA

0b35cc9

Fix ui test

6426f80

Fix ui test again

0fe5334

Modify .puml files

acbc70c

Modify ArchitectureSequenceGrouped.puml

66c752c

Re-export png files

f74ab22

Modify DeveloperGuide.md

d900339

RCPilot1604 mentioned this pull request Nov 7, 2024

Clean up DG AY2425S1-CS2113-W13-4/tp#130

Merged

RCPilot1604 and others added 16 commits November 7, 2024 14:28

Merge pull request #130 from RCPilot1604/branch-DG-cleanup

ef209b0

Clean up DG

Merge pull request #116 from ThienDuc3112/calcGPA

87373a9

Add calcGPA command

Update DG manual testing for Storage

25cf76a

Update GradeStorage uml

76f5459

Merge branch 'master' of https://github.com/AY2425S1-CS2113-W13-4/tp

6b2ad46

Merge branch 'master' of https://github.com/yeekian/tp into branch-re…

f299b86

…factor

Add print individual student grade list

913bb1c

Reformat output messages for grade list

abfc183

Modify AddComponentCommand to include check for total weighting

04407f4

Modify AddComponentCommand to check for total weighting

9e5b7bb

Modify Commons to add max weighting error message

d70250f

Fix Component.equals()

e1a8c4d

Update IO redirection testing

c4611dd

Modify tests

540a4b0

Fix checkstyle and format

a104e0f

RCPilot1604 and others added 30 commits November 12, 2024 09:51

Refine add_grade UG

f301ff4

Merge pull request #228 from RCPilot1604/branch-UG-update

5c89436

Branch UG update

Update UserGuide.md

6cb51a3

Merge pull request #229 from AY2425S1-CS2113-W13-4/RCPilot1604-patch-1

b9447ac

Update UserGuide.md

Update UserGuide.md

73690b3

Merge pull request #230 from AY2425S1-CS2113-W13-4/RCPilot1604-patch-2

fb81216

Update UserGuide.md

Fix update_component

7f55a10

Merge branch 'master' into fix-update-component-error

7ffd86f

Merge pull request #231 from RCPilot1604/fix-update-component-error

f0303e9

Update AboutUs.md

09e3569

Merge pull request #1 from RCPilot1604/RCPilot1604-patch-1

d3701a2

Update AboutUs.md

Merge pull request #232 from RCPilot1604/fix-PPP-link

418ed5a

Fix ppp link

Add Javadoc to AddStudentCommand

c865800

Add Javadoc to DeleteComponentCommand

1cb8ff0

Add javadoc to DeleteStudentCommand

1033ca4

Add javadoc to FindStudentCommand

0a1887e

Merge pull request #233 from RCPilot1604/branch-more-javadoc

9dcec6d

Branch more javadoc

Update PPP

cdb42d1

Refactor PPP

861d5ee

Merge pull request #234 from yeekian/master

171ccda

Update yeekian PPP

Update PPP trungbui32.md

a5c493f

Bugfix for incorrect error message

e82f2ee

Update PPP

e6e932f

Merge pull request #236 from TrungBui32/updatedppp

8d8f676

Updatedppp

Merge pull request #235 from RCPilot1604/branch-emergency-bugfix

a931475

Bugfix for incorrect error message

Add author tags

7d8de50

Merge pull request #237 from jinzihan2002/Zihan-authortag

44cac67

Add author tags for GradeStorage

Modify puml diagram

da02a44

Fix puml diagram (CRLF to LF)

a881cbf

Merge pull request #239 from RCPilot1604/branch-fix-reposense

d8478b2

Branch fix reposense

		The following sequence diagrams depict the exact steps involved in the `AddStudentCommand`:

		![AddStudentCommand.png](diagrams/AddStudentCommand.png)

		All commands follow the sequence as described in the diagram below:

		![ArchitectureSequenceGrouped.png](diagrams%2FArchitectureSequenceGrouped.png)


		Likewise, the sequence diagram for `DeleteStudentCommand` is as follows:

		![DeleteStudentCommand.png](diagrams%2FDeleteStudentCommand.png)


		The sequence diagram of the DeleteGradeCommand is shown below.

		![AddGradeCommand.png](diagrams%2FAddGradeCommand.png)


		![GradeStorage.png](diagrams/GradeStorage.png)

[CS2113-W13-4] TutorLink #13

Are you sure you want to change the base?

[CS2113-W13-4] TutorLink #13

Conversation

RCPilot1604 commented Sep 19, 2024

Mahesh1772 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakultbottle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wongwh2002 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahchow03 left a comment

Choose a reason for hiding this comment

sarahchow03 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

sarahchow03 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahchow03 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

sarahchow03 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

sarahchow03 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

sarahchow03 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

sarahchow03 Oct 31, 2024 •

edited

Loading

sarahchow03 Oct 31, 2024 •

edited

Loading

sarahchow03 Oct 31, 2024 •

edited

Loading

sarahchow03 Oct 31, 2024 •

edited

Loading

sarahchow03 Oct 31, 2024 •

edited

Loading

sarahchow03 Oct 31, 2024 •

edited

Loading