Refactor StudentLogic::enrollStudents(String, String, boolean) #6557 #6609

boxin-yang · 2017-02-14T18:07:29Z

…t list

Outline of Solution

studentList could have been built in getInvalidityInfoInEnrollLines(String, String) . Reuse the logic in getInvalidityInfoInEnrollLines to build the studentList to prevent building it twice.(and the StudentAttributes built in enrollStudents(String, String, boolean) uses String that is not sanitized. Reusing getInvalidityInfoInEnrollLines solves the issue).

…t list

junhaoyap · 2017-02-16T10:08:41Z

@boxin-yang there are some errors (checkstlye errors) that is causing the build to fail, could you please update it and I will set it for review when the build passes? In the mean time I am setting it to be ongoing

… function getInvalidityInfoInEnrollLines

…ion of parameter studentList

junhaoyap · 2017-03-01T15:12:13Z

@boxin-yang it is not necessary to merge from "update branch" especially if the build is failing! Try to fix it locally on your machine before attempting it again, it helps to not clog up the build machine

boxin-yang · 2017-03-01T15:27:26Z

@junhaoyap I see. Will run the local test and Checkstyle before I push in the future. Thx for the advice.

junhaoyap · 2017-03-03T13:00:47Z

@boxin-yang seems like it has passed this time! Is this ready to be reviewed?

boxin-yang · 2017-03-03T13:19:46Z

@junhaoyap Yes this PR is ready for Review

junhaoyap

Please be careful with language implementations, Java is pass by value so studentList will remain empty at the end of line 305

Read: http://stackoverflow.com/questions/40480/is-java-pass-by-reference-or-pass-by-value

junhaoyap · 2017-03-03T14:04:50Z

Without test coverage that tests for both correct / incorrect cases of what I described, I can't be 100% sure that that is what's happening, I am basing it off of the pass-by-value rule, whether it is right or wrong, I think the best route from here is to write a test case that covers it

cc @wkurniawan07 to confirm the pass-by-value effects or whether I'm just conjuring crazy stuff

junhaoyap · 2017-03-03T14:09:29Z

It is me being crazy after all, I got confused switching around from languages to languages every day

Please read instead: http://stackoverflow.com/questions/7127530/does-array-changes-in-method

There is a difference between primitives and objects :-)

junhaoyap

Other than my crazy self rambling, we are good for this :p

junhaoyap · 2017-03-03T14:10:46Z

cc @damithc for second checking

wkurniawan07 · 2017-03-03T18:33:38Z

@junhaoyap you'd be right if the parameter is a primitive, e.g. String, Integer, but Objects are passed by reference.
@boxin-yang nevertheless, this is not a really good way to solve the issue. I'll explain why soon.

…github.com/greed-is-good/teammates into 6557-refactor-student-logic-enroll-student

boxin-yang · 2017-03-08T14:12:06Z

@junhaoyap @damithc Ready for review

damithc

A few more things to fix. Almost there.

damithc · 2017-03-08T14:20:07Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-        for (int i = 0; i < invalidInfo.size(); i++) {
-            assertEquals(expectedInvalidInfo.get(i), invalidInfo.get(i));
+    private String getExceptionMessageOnCreatingStudentsList(String enrollLines, String courseId) {
+        String invalidInfoString = null; // null will be returned if no exception is thrown


This is incorrect. Line 593 will terminate the execution if no exception is thrown.

damithc · 2017-03-08T14:22:26Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java


-        for (int i = 0; i < invalidInfo.size(); i++) {
-            assertEquals(expectedInvalidInfo.get(i), invalidInfo.get(i));
+    private String getExceptionMessageOnCreatingStudentsList(String enrollLines, String courseId) {


Write a header comment explaining what this method does. Mention that it assumes an exception will be thrown and cause a test failure if it doesn't.

damithc · 2017-03-08T14:25:02Z

src/main/java/teammates/logic/core/StudentsLogic.java

@@ -592,12 +572,13 @@ private StudentEnrollDetails enrollStudent(StudentAttributes validStudentAttribu
    }

    /* All empty lines or lines with only white spaces will be skipped.
-     * The invalidity info returned are in HTML format.
+     * The invalidity info is in HTML format.


As we are changing the method behavior, we can also fix this header comment. It should start with /**. Describe when does it throws the exception.

Can you push the changes, we don't see the change for /**

Sorry forgot to push. Just pushed

damithc · 2017-03-08T14:25:45Z

src/main/java/teammates/logic/core/StudentsLogic.java

     */
-    private List<String> getInvalidityInfoInEnrollLines(String lines, String courseId) throws EnrollException {
+    public List<StudentAttributes> createAndValidateStudents(String lines, String courseId) throws EnrollException {


I don't think Validate should be in the method name. Methods are supposed to validate data anyway.

Well, this new method combined the part on validation and creating studentList together(done separately in the original code). Also, mentioning Validate might help with understanding the purpose of this function. Meanwhile, I have explained the exception in the header comment to mention about validating. Any suggestion on this ?@wkurniawan07

I think @damithc nailed it, this method says throws EnrollException which pretty much tells us something is being "validated", just createStudents is good enough

Ok will update the name

junhaoyap · 2017-03-08T15:23:34Z

I believe we will be blocked again in terms of merging again due to accountsDB failing once we merge master in..

…MessageOnCreatingStudentsList

boxin-yang

@damithc Ready for review

boxin-yang · 2017-03-08T15:30:13Z

src/main/java/teammates/logic/core/StudentsLogic.java

@@ -592,12 +572,13 @@ private StudentEnrollDetails enrollStudent(StudentAttributes validStudentAttribu
    }

    /* All empty lines or lines with only white spaces will be skipped.
-     * The invalidity info returned are in HTML format.
+     * The invalidity info is in HTML format.


Sorry forgot to push. Just pushed

boxin-yang · 2017-03-08T15:32:57Z

src/main/java/teammates/logic/core/StudentsLogic.java

     */
-    private List<String> getInvalidityInfoInEnrollLines(String lines, String courseId) throws EnrollException {
+    public List<StudentAttributes> createAndValidateStudents(String lines, String courseId) throws EnrollException {


Ok will update the name

boxin-yang · 2017-03-09T11:06:09Z

@damithc Updated the param format in method comments as we discussed during class. Ready for review

damithc

Some comments added.

damithc · 2017-03-09T11:37:36Z

src/main/java/teammates/logic/core/StudentsLogic.java

-    /* All empty lines or lines with only white spaces will be skipped.
-     * The invalidity info is in HTML format.
+    /**
+     * This method builds the studentList from user input lines and check the validity of each student instance created.


and check the validity of each student instance created is not needed because it is implied by the @throws clause description.

damithc · 2017-03-09T11:38:02Z

src/main/java/teammates/logic/core/StudentsLogic.java

+    /**
+     * This method builds the studentList from user input lines and check the validity of each student instance created.
+     *
+     * @param lines  the enrollment lines entered by the instructor.All empty lines or lines with only white


There should be a space after .

The second sentence is about the method behavior, not about the parameter. It should be moved up.

damithc · 2017-03-09T11:40:19Z

src/main/java/teammates/logic/core/StudentsLogic.java

+     *
+     * @param lines  the enrollment lines entered by the instructor.All empty lines or lines with only white
+     *         spaces will be skipped.
+     * @throws EnrollException  if any of the student instances created by the enrollment line is invalid.The invalidity


by the enrollment line is not needed.
Add space after .
It's not clear what this invalidity info you are referring to.

Also, it is not clear if the exception is thrown for the first invalid student, or for all invalid students.

damithc · 2017-03-09T11:41:59Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

@@ -586,10 +586,18 @@ public void testEnrollLinesChecking() throws Exception {
        assertEquals(expectedInvalidInfoString, invalidInfoString);
    }

+    /**
+     * This method returns the Exception message thrown when trying to build StudentList from invalid enrollment line.


No need for This method

damithc · 2017-03-09T11:43:37Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

    private String getExceptionMessageOnCreatingStudentsList(String enrollLines, String courseId) {
-        String invalidInfoString = null; // null will be returned if no exception is thrown


The parameter should be invalidEnrollLines?

yup it would be clearer

damithc · 2017-03-09T11:44:33Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

+     * signalFailureToDetectException().
+     *
+     * @param enrollLines is assumed to be invalid
+     * @return invalidInfoString in the Enrollment Exception in html format


invalidInfoString is a local variable. Should not be mentioned here.

damithc · 2017-03-09T11:45:10Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

@@ -586,10 +586,18 @@ public void testEnrollLinesChecking() throws Exception {
        assertEquals(expectedInvalidInfoString, invalidInfoString);
    }

+    /**
+     * This method returns the Exception message thrown when trying to build StudentList from invalid enrollment line.
+     * The method assumes that an Enrollment Exception is thrown, else the method fails with


Enrollment Exception should not have a space in the middle?

…t and createStudents

…github.com/greed-is-good/teammates into 6557-refactor-student-logic-enroll-student

boxin-yang · 2017-03-09T12:10:26Z

@damithc Thx for the review and suggestion. Learnt a lot. Just made some updates and is ready for review

ablyx · 2017-03-09T12:29:19Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-     * This method returns the Exception message thrown when trying to build StudentList from invalid enrollment line.
-     * The method assumes that an Enrollment Exception is thrown, else the method fails with
-     * signalFailureToDetectException().
+     * Returns the Exception message thrown when trying to build StudentList from invalid enrollment line.


Since this is describing what is returned, it can be placed in @return.
Would @return EnrollException message thrown due to invalid enrollment line in html format be better?

The return here is more like a 'get' for method description. @return param description is to emphasize html format.

Without using 'return' here the sentence is complete and hard to understand. This method is in a way a get method, so inevitably I have to use either return or get in my method description.

ablyx

minor nit

ablyx · 2017-03-09T12:29:49Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-     * @param enrollLines is assumed to be invalid
-     * @return invalidInfoString in the Enrollment Exception in html format
+     * @param invalidEnrollLines  is assumed to be invalid
+     * @return the EnrollException message in html format


the is unnecessary

thx for pointing out! done changing :)

damithc

Much better now. Some more minor suggestions.

damithc · 2017-03-09T13:59:12Z

src/main/java/teammates/logic/core/StudentsLogic.java

+     *
+     * @param lines the enrollment lines entered by the instructor.
+     * @throws EnrollException if some of the student instances created are invalid. The exception message contains
+     *         invalidity info for all invalid student instances in HTML format.


Looks good.

damithc · 2017-03-09T14:06:22Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-        for (int i = 0; i < invalidInfo.size(); i++) {
-            assertEquals(expectedInvalidInfo.get(i), invalidInfo.get(i));
+    /**
+     * Returns the Exception message thrown when trying to build StudentList from invalid enrollment line.


StudentList is an undefined term (i.e. reader has no idea what it means). I think you can instead explain that the method calls createStudents(...) on the studentLogic object.

damithc · 2017-03-09T14:06:25Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

+     * The method assumes that an EnrollException is thrown, else the method fails with signalFailureToDetectException().
+     *
+     * @param invalidEnrollLines is assumed to be invalid
+     * @return EnrollException message in html format


in html format in not relevant here. The method simply returns whatever in the exception object.

damithc · 2017-03-09T14:07:52Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-        for (int i = 0; i < invalidInfo.size(); i++) {
-            assertEquals(expectedInvalidInfo.get(i), invalidInfo.get(i));
+    /**
+     * Returns the Exception message thrown when trying to build StudentList from invalid enrollment line.


It's not correct to say Exception message thrown. The said method throws an exception, not a message.

boxin-yang · 2017-03-09T14:25:08Z

@damithc Ready for review with suggested change. @ablyx I just updated the method description again. Do you think it is clearer this time round? (I have to mention return enrollException thrown... to explain what the method is doing, can't think of a better way to describe the method)

damithc

A few more nits

damithc · 2017-03-09T14:33:47Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-        for (int i = 0; i < invalidInfo.size(); i++) {
-            assertEquals(expectedInvalidInfo.get(i), invalidInfo.get(i));
+    /**
+     * Returns the EnrollException thrown when trying to call createStudents method on studentsLogic object with invalid


Returns the EnrollException thrown doesn't match the return type of the method. How about Returns the error message of the EnrollException thrown ...?

damithc · 2017-03-09T14:36:16Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-            assertEquals(expectedInvalidInfo.get(i), invalidInfo.get(i));
+    /**
+     * Returns the EnrollException thrown when trying to call createStudents method on studentsLogic object with invalid
+     * enrollment lines. The method assumes that an EnrollException is thrown, else the method fails with


Should be with {@code invalidEnrollLines} (to indicate that we are referring to the parameter, not any invalid enrollment lines).

damithc · 2017-03-09T14:38:48Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

-        for (int i = 0; i < invalidInfo.size(); i++) {
-            assertEquals(expectedInvalidInfo.get(i), invalidInfo.get(i));
+    /**
+     * Returns the EnrollException thrown when trying to call createStudents method on studentsLogic object with invalid


Use {@code ...} when referring to code elements. e.g. {@code studentsLogic}

There is a javadoc syntax to refer to another method. I'm saying this in reference to createStudents in the comment.

I googled around and I think I should use {@link} to refer to method. Did the same for signalFailureToDetectException

damithc · 2017-03-09T14:41:17Z

src/test/java/teammates/test/cases/logic/StudentsLogicTest.java

+     * signalFailureToDetectException().
+     *
+     * @param invalidEnrollLines is assumed to be invalid
+     * @return EnrollException message


Probably no need if the same info is in the overview statement (line 590)

getExceptionMessageOnCreatingStudentsList and createStudents

boxin-yang · 2017-03-09T15:13:47Z

@damithc Googled around and learnt to use {@link} and {@code}. Also updated the method comments for createStudents

damithc

All good now :-)

reuse getInvalidityInfoInEnrollLines in student logic to build studen…

07c08e7

…t list

junhaoyap added the s.Ongoing The PR is being worked on by the author(s) label Feb 16, 2017

boxin-yang added 7 commits March 1, 2017 18:55

Merge branch 'master' into 6557-refactor-student-logic-enroll-student

c6656e2

update StudentsLogicTest corresponding to the change of parameter for…

370a9d4

… function getInvalidityInfoInEnrollLines

update javadoc of getInvalidityInfoInEnrollLines to explain the addit…

bba0975

…ion of parameter studentList

fix white space before comma as warned by CheckStyle

14df30e

Merge branch 'master' into 6557-refactor-student-logic-enroll-student

f00da88

delete white space

15c856e

Merge branch 'master' into 6557-refactor-student-logic-enroll-student

3e7b2d1

junhaoyap self-assigned this Mar 3, 2017

junhaoyap self-requested a review March 3, 2017 13:39

junhaoyap added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 3, 2017

junhaoyap requested changes Mar 3, 2017

View reviewed changes

junhaoyap added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Mar 3, 2017

junhaoyap added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 3, 2017

junhaoyap approved these changes Mar 3, 2017

View reviewed changes

junhaoyap added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Mar 3, 2017

Merge branch '6557-refactor-student-logic-enroll-student' of https://…

28b8d11

…github.com/greed-is-good/teammates into 6557-refactor-student-logic-enroll-student

damithc requested changes Mar 8, 2017

View reviewed changes

boxin-yang added 2 commits March 8, 2017 23:26

add in method comments for createAndValidateStudents and getException…

6c756c9

…MessageOnCreatingStudentsList

rename createAndValidateStudents to createStudents

80932bc

boxin-yang commented Mar 9, 2017

View reviewed changes

boxin-yang added 2 commits March 9, 2017 12:46

Refactor the line wrapping in createStudents method comment

0f87fe1

Merge branch 'master' into 6557-refactor-student-logic-enroll-student

3efadff

damithc requested changes Mar 9, 2017

View reviewed changes

boxin-yang added 2 commits March 9, 2017 20:07

refactor method comments for getExceptionMessageOnCreatingStudentsLis…

1e38b74

…t and createStudents

Merge branch '6557-refactor-student-logic-enroll-student' of https://…

7b833fb

…github.com/greed-is-good/teammates into 6557-refactor-student-logic-enroll-student

ablyx reviewed Mar 9, 2017

View reviewed changes

ablyx requested changes Mar 9, 2017

View reviewed changes

remove the in getExceptionMessageOnCreatingStudentsList

e04eb66

damithc requested changes Mar 9, 2017

View reviewed changes

refactor getExceptionMessageOnCreatingStudentsList method comment

b67af47

damithc requested changes Mar 9, 2017

View reviewed changes

Include {@link} {@code} tag to method comments for

6a2c7a3

getExceptionMessageOnCreatingStudentsList and createStudents

damithc approved these changes Mar 9, 2017

View reviewed changes

damithc added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Mar 9, 2017

ablyx approved these changes Mar 9, 2017

View reviewed changes

wkurniawan07 merged commit d8ae0a3 into TEAMMATES:master Mar 9, 2017

wkurniawan07 added the e.2 label Mar 9, 2017

wkurniawan07 added this to the V5.99 milestone Mar 9, 2017

		private String getExceptionMessageOnCreatingStudentsList(String enrollLines, String courseId) {
		String invalidInfoString = null; // null will be returned if no exception is thrown

Refactor StudentLogic::enrollStudents(String, String, boolean) #6557 #6609

Refactor StudentLogic::enrollStudents(String, String, boolean) #6557 #6609

Conversation

boxin-yang commented Feb 14, 2017

junhaoyap commented Feb 16, 2017

junhaoyap commented Mar 1, 2017

boxin-yang commented Mar 1, 2017

junhaoyap commented Mar 3, 2017

boxin-yang commented Mar 3, 2017

junhaoyap left a comment

Choose a reason for hiding this comment

junhaoyap commented Mar 3, 2017

junhaoyap commented Mar 3, 2017

junhaoyap left a comment

Choose a reason for hiding this comment

junhaoyap commented Mar 3, 2017

wkurniawan07 commented Mar 3, 2017

boxin-yang commented Mar 8, 2017

damithc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junhaoyap commented Mar 8, 2017

boxin-yang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boxin-yang commented Mar 9, 2017

damithc 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

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

boxin-yang commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ablyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc 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

boxin-yang commented Mar 9, 2017

damithc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment