fix: refactor to improve readability of main_page function #230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Gary Kim Bot <bot@garykim.dev>
By delegating the tasks to helper functions, the code becomes marginally easier to browse through. Better function name could be selected, however. getStudentName has better readability than what was previously used, and conveys its function better. s1col and s2col were renamed to sem1_col and sem2_col respectively. This is because "sem" is more understandable than "s". Assigning values to sem1_col and sem2_col was put into a function. Checking whether it was the second semester was also put into a function. Initially both of these tasks were mashed together, but separating it into two functions hopefully provides more clarity and modularity. I added newlines to the import statement from helpers because I initially put the getSemesterCols and isSecondSemester functions in there, but those changes aren't necessary. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
When reading the code in the main_page function, a headache can be
caused from the amount of jQuery code that has to be traversed. In
continuation of solving this problem, I have moved some of the code into
more descriptive functions.
The showCurrentGPA function is more descriptive than random jQuery.
The showfirstSemGPA function is also more descriptive than a fetch for
the HTML and a lot of HTML parsing and then finally displaying the GPA.
The getFirstSemCourses function is *hopefully* more descriptive than
fetch("https://powerschool.sas.edu.sg/guardian/termgrades.html").
There's probably a better function name for this.
By moving some of the code into the main_page function, it hopefully
creates better readability of the main_page function.
Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
Again, aiming for clarity in the main_page function. Wrapping the if statement's very long jQuery parse into a function name that's in English makes the code easier on the eyes. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
getCourses is a bit misleading considering it also adds a new class grade, however, conveys the point better than it collects the user's courses. I additionally made the variables at the start of main_page constant because there is no reason they should change after the function call. Making these changes improves readability of the code, better conveying what portions of it are doing. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
Again just moving code into functions for better readability. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
Easier to comprehend with a single function name rather than a series of DOM modifications. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
`sem1_col` and `sem2_col` were initially undefined in this function. Adding parameters for them and passing them as arguments fixes this. I additionally fixed whitespace issues identified by the CI/CD pipeline. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
There were extra spaces at the end of lines, and some tabs were missing before some lines. This resolves that issue. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
gary-kim
reviewed
Nov 11, 2020
Gary recommended this change. A lot more condensed. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
Suhas-13
requested changes
Nov 16, 2020
Suhas-13
reviewed
Nov 16, 2020
addHypoAssignment takes in an argument for the current final percent of the student, but no argument was actually passed. This commit fixes that by actually passing the argument. This bug was caught by Suhas. Signed-off-by: Lucas Sta Maria <lucas.stamaria@gmail.com>
Suhas-13
approved these changes
Nov 16, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When initially browsing through the codebase, it was very difficult to grasp what all the lines of code were doing. I took the time to move verbose blocks of code into functions whose names hopefully describe what they were doing. By representing these blocks with single-line functions, it makes it easier to understand what the
main_pagefunction is trying to do, and thus less intimidating to contribute to.I additionally rebased my commits to make their subject lines more consistent with one another, and in doing so, accidentally made myself a contributor to the
9eceaf4commit, so ignore that. My commit messages further detail my line of reasoning for specific chunks in the code.