CSC/ECE 517 Fall 2014/OSS E1467 rsv: Difference between revisions
(20 intermediate revisions by 3 users not shown) | |||
Line 2: | Line 2: | ||
'''Project name: [https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit E1467: Expertiza - Refactoring LeaderBoard model]'''<ref>[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit E1467: Expertiza - Refactoring LeaderBoard model]</ref> | '''Project name: [https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit E1467: Expertiza - Refactoring LeaderBoard model]'''<ref>[https://docs.google.com/document/d/1FZCL9KWSdVNsX9BowuZ3gxbCOJoiWX-GVLctSZei3No/edit E1467: Expertiza - Refactoring LeaderBoard model]</ref> | ||
Our project is to refactor the code in the ''"Leaderboard"'' functionality of the web application [https://github.com/expertiza/expertiza Expertiza]<ref>[https://github.com/expertiza/expertiza Expertiza]</ref>. The Expertiza project is a system to create reusable learning objects through peer review. The leaderboard functionality is to show top 3 individual scorers in each questionnaire type ( | Our project is to refactor the code in the ''"Leaderboard"'' functionality of the web application [https://github.com/expertiza/expertiza Expertiza]<ref>[https://github.com/expertiza/expertiza Expertiza]</ref>. The Expertiza project is a system to create reusable learning objects through peer review. The leaderboard functionality is to show top 3 individual scorers in each questionnaire type (''ReviewQuestionnaire'', ''AuthorFeedbackQuestionnaire'', ''MetareviewQuestionnaire'' and ''TeammateReviewQuestionnaire''), in each course of the logged-in user. The leaderboard also shows the current standing of the logged in user under personal achievements. | ||
<div id="projectLinks" style="float: right;"> | |||
< | [[File:expertiza_logo.jpg|center]] | ||
{| class="wikitable | {| class="wikitable" | ||
| colspan="3" align="center" | Expertiza | | colspan="3" align="center" | '''Expertiza project links''' | ||
|- | |||
| '''Refactored Instance''' | |||
| http://152.46.18.78:3000/ | |||
| rowspan="2" | '''username= "user480"<br />password= "password"''' | |||
|- | |||
| '''Original Instance''' | |||
| http://152.46.18.78:3001/ | |||
|- | |- | ||
| | | '''Repository link''' | ||
| | | colspan="3" | https://github.com/sanjeevs/expertiza <ref>[https://github.com/sanjeevs/expertiza Forked repository]</ref> | ||
|- | |- | ||
| | | '''Pull request''' | ||
| | | colspan="3" | https://github.com/expertiza/expertiza/pull/444 <ref>[https://github.com/expertiza/expertiza/pull/444 Pull request]</ref> | ||
|} | |} | ||
</ | </div> | ||
__TOC__ | __TOC__ | ||
Line 30: | Line 37: | ||
4. Seperate out computation of CS entries(metric) and refactor it to be more modular and elegant. | 4. Seperate out computation of CS entries(metric) and refactor it to be more modular and elegant. | ||
5. Refactor <code style="background:#F0F0FF"> getParticipantEntriesInAssignmentList </code> method. Its very complex <code>(Complexity=142)</code>. | 5. Refactor <code style="background:#F0F0FF"> getParticipantEntriesInAssignmentList </code> method. Its very complex <code style="background:#F0F0FF">(Complexity=142)</code>. | ||
6. Refactor <code style="background:#F0F0FF"> addEntryToCSHash </code> according to your new metric method. | 6. Refactor <code style="background:#F0F0FF"> addEntryToCSHash </code> according to your new metric method. | ||
Line 45: | Line 52: | ||
|- style="vertical-align:top;" | |- style="vertical-align:top;" | ||
|- | |- | ||
|''''' 1 ''''' | |style="text-align:center;"|''''' 1 ''''' | ||
| <code> getParticipantEntriesInAssignmentList(assignmentList) </code> | | <code> getParticipantEntriesInAssignmentList(assignmentList) </code> | ||
|This method is responsible for calculating the leaderboard of all the participants associated with assignments in given assignment list. | |This method is responsible for calculating the leaderboard of all the participants associated with assignments in given assignment list. | ||
|- | |- | ||
|''''' 2 ''''' | |style="text-align:center;"|''''' 2 ''''' | ||
| <code> extractPersonalAchievements(csHash, courseIdList, userId) </code> | | <code> extractPersonalAchievements(csHash, courseIdList, userId) </code> | ||
| This method is responsible for calculating personal aggregated score. It also calculates the ranking of currently logged in user against total users associated with the same set of courses as that of current user. | | This method is responsible for calculating personal aggregated score. It also calculates the ranking of currently logged in user against total users associated with the same set of courses as that of current user. | ||
|- | |- | ||
|''''' 3 ''''' | |style="text-align:center;"|''''' 3 ''''' | ||
| <code> addEntryToCSHash(qtypeHash, qtype, userid, csEntry, courseid) </code> | | <code> addEntryToCSHash(qtypeHash, qtype, userid, csEntry, courseid) </code> | ||
| This method is called internally from <code style="background:F0F0FF"> Leaderboard.getParticipantEntriesInAssignmentList </code>. This method aggregates score of a user grouped by course id, further grouped by questionnaire type. | | This method is called internally from <code style="background:#F0F0FF"> Leaderboard.getParticipantEntriesInAssignmentList </code>. This method aggregates score of a user grouped by course id, further grouped by questionnaire type. | ||
|- | |- | ||
|} | |} | ||
=Changes in Model = | |||
In the OSS project, we have refactored these methods along with other smaller methods in | In the OSS project, we have refactored these methods along with other smaller methods in | ||
<code style="background:F0F0FF"> Leaderboard.rb </code>, <code style="background:F0F0FF"> Leaderboard_helper.rb </code>, <code style="background:F0F0FF"> Leaderboard_controller.rb </code> and associated views files. We have also refactored ambiguous variable and method names. | <code style="background:#F0F0FF"> Leaderboard.rb </code>, <code style="background:#F0F0FF"> Leaderboard_helper.rb </code>, <code style="background:#F0F0FF"> Leaderboard_controller.rb </code> and associated views files. We have also refactored ambiguous variable and method names. | ||
We have keenly focussed in reducing the database calls, loops and redundant storage and computation. We have refactored many files related to Leaderboard implementation leading to reduction in overall complexity of the feature. | We have keenly focussed in reducing the database calls, loops and redundant storage and computation. We have refactored many files related to Leaderboard implementation leading to reduction in overall complexity of the feature. | ||
Also, there was a requirement in the project, that we should come up with an efficient way to search for participants based on assignment ids. However, upon investigation, we found that scores stored in table ScoreCache, gives us revieweeId which is either participantId or teamId. Therefore, we have a method <code> getAssignmentMapping </code>, which creates a mapping of participant and team with corresponding assignment. This is very useful while computing leaderboard. | |||
Changes made in the methods of model <code> '''leaderboard.rb''' </code> | Changes made in the methods of model <code> '''leaderboard.rb''' </code> | ||
Line 74: | Line 83: | ||
! style="width:43%;"|Reason For Change | ! style="width:43%;"|Reason For Change | ||
|- style="vertical-align:top;" | |- style="vertical-align:top;" | ||
|''''' 1 ''''' | |style="text-align:center;"|''''' 1 ''''' | ||
| <code> getIndependantAssignments </code> | | <code> getIndependantAssignments </code> | ||
| Separate db call for each user assignment. | | Separate db call for each user assignment. | ||
| Single db call with an array of assignment ids. | | Single db call with an array of assignment ids. | ||
|- | |- | ||
|''''' 2 ''''' | |style="text-align:center;"|''''' 2 ''''' | ||
| <code> getParticipantEntriesInCourses </code> | | <code> getParticipantEntriesInCourses </code> | ||
| Confusing code. Appends each entry to the end of list | | Confusing code. Appends each entry to the end of list | ||
| Used concat to clarify that we are adding the independent and other assignments in courses. | | Used concat to clarify that we are adding the independent and other assignments in courses. | ||
|- | |- | ||
|''''' 3 ''''' | |style="text-align:center;"|''''' 3 ''''' | ||
| <code> sortHash </code> | | <code> sortHash </code> | ||
| in place changes. | | in place changes. | ||
| Returns a new deep copy of the updated hash. | | Returns a new deep copy of the updated hash. | ||
|- | |- | ||
|''''' 4 ''''' | |style="text-align:center;"|''''' 4 ''''' | ||
| <code> extractPersonalAchievements </code> | | <code> extractPersonalAchievements </code> | ||
| Confusing code with extra data base calls. | | Confusing code with extra data base calls. | ||
| This method is refactored to use only 1 database call unlike several within the loop in its prior implementation. We have also removed redundant code computation and made the logic easier to understand. The overall complexity of this method is reduced from 72 to 35. | | This method is refactored to use only 1 database call unlike several within the loop in its prior implementation. We have also removed redundant code computation and made the logic easier to understand. The overall complexity of this method is reduced from 72 to 35. | ||
|- | |- | ||
|''''' 5 ''''' | |style="text-align:center;"|''''' 5 ''''' | ||
| <code> addEntryToCSHash </code> | | <code> addEntryToCSHash </code> | ||
| Confusing name and hard to follow logic. | | Confusing name and hard to follow logic. | ||
| The new method name is <code> addScoreToResultantHash </code>. As mentioned earlier, this is called internally by <code> getParticipantEntriesInAssignmentList </code>. The complexity of this method is reduced from 67 to 39 . | | The new method name is <code style="background:#F0F0FF"> addScoreToResultantHash </code>. As mentioned earlier, this is called internally by <code style="background:#F0F0FF"> getParticipantEntriesInAssignmentList </code>. The complexity of this method is reduced from 67 to 39 . | ||
|- | |- | ||
|''''' 6 ''''' | |style="text-align:center;"|''''' 6 ''''' | ||
| <code> getParticipantEntriesInAssignmentList </code> | | <code> getParticipantEntriesInAssignmentList </code> | ||
| This method was the most complex method in the class. | | This method was the most complex method in the class. | ||
| The new refactored method name is <code> getParticipantsScore </code>. The method was refactored on various aspects like reducing database calls drastically, removing redundant code computation, redundant data storage in complex group of hashes and series of conditional statements. We would like to mention that previously, the method had 10 database calls within loop and 1 outside loop. After refactoring, the new method has just 1 database call within loop and 3 outside loops. Also, the overall code complexity is reduced from 142 to 64. | | The new refactored method name is <code style="background:#F0F0FF"> getParticipantsScore </code>. The method was refactored on various aspects like reducing database calls drastically, removing redundant code computation, redundant data storage in complex group of hashes and series of conditional statements. We would like to mention that previously, the method had 10 database calls within loop and 1 outside loop. After refactoring, the new method has just 1 database call within loop and 3 outside loops. Also, the overall code complexity is reduced from 142 to 64. | ||
|- | |- | ||
|} | |} | ||
Line 117: | Line 126: | ||
! style="width:43%;"|Reason For Change | ! style="width:43%;"|Reason For Change | ||
|- style="vertical-align:top;" | |- style="vertical-align:top;" | ||
|''''' 1 ''''' | |style="text-align:center;"|''''' 1 ''''' | ||
| <code> userIsInstructor </code> | | <code> userIsInstructor </code> | ||
| Rewrote to reduce the number of database calls. | | Rewrote to reduce the number of database calls. | ||
| This method was refactored to reduce multiple database calls. 4 database calls was replaced by single call. | | This method was refactored to reduce multiple database calls. 4 database calls was replaced by single call. | ||
|- | |- | ||
|''''' 2 ''''' | |style="text-align:center;"|''''' 2 ''''' | ||
| <code> studentInWhichCourses </code> | | <code> studentInWhichCourses </code> | ||
| Rewrote to reduce the number of database calls. | | Rewrote to reduce the number of database calls. | ||
| This method was refactored to reduce multiple database calls and remove unnecessary loops. 1 database call outside and 1 within a loop was replaced by a |single call. | | This method was refactored to reduce multiple database calls and remove unnecessary loops. 1 database call outside and 1 within a loop was replaced by a |single call. | ||
|- | |- | ||
|''''' 3 ''''' | |style="text-align:center;"|''''' 3 ''''' | ||
| <code> getTop3Leaderboards </code> | | <code> getTop3Leaderboards </code> | ||
| Dead code | | Dead code | ||
Line 138: | Line 147: | ||
and variables, adding proper comments etc and minor code refactoring. Please refer to our forked | and variables, adding proper comments etc and minor code refactoring. Please refer to our forked | ||
github repository to view those changes. | github repository to view those changes. | ||
=Design Pattern= | |||
Leaderboard is a separate implementation which includes reading existing scores for the sole purpose of creating the leaders in different categories. As the task is more related to calculate and manipulate data, it was not feasible to implement any design pattern. We reduced the complexity and redundancy of database calls by reducing database calls from 625 to 111 to do the same task. | |||
=Testing= | =Testing= | ||
On VCL session there are 2 instances running. On <code> port 3001 </code> the original instance is setup and on <code> port 3000 </code> the refactored instance is setup. | On VCL session there are 2 instances running. On <code> port 3001 </code> the original instance is setup and on <code> port 3000 </code> the refactored instance is setup. Login by any user and navigate to ''Home > Leaderboard'' to test the functionality. | ||
In below images we have shown that the output after refactoring is same. <b>We changed the order in which the output is displayed to make it more coherent.</b> | In below images we have shown that the output after refactoring is same. <b>We changed the order in which the output is displayed to make it more coherent.</b> | ||
'''Note for testing :''' It is recommended that in order to test the leaderboard functionality by any user who has participated in several assignments, some of them which is associated with any course and there are other existing users who have participated in the same assignment.<br /> | '''Note for testing :''' It is recommended that in order to test the leaderboard functionality by any user who has participated in several assignments, some of them which is associated with any course and there are other existing users who have participated in the same assignment.<br /> | ||
[[# | [[#projectLinks|'''Expertiza Test instances links''']] | ||
{| class="wikitable" | {| class="wikitable" | ||
Line 165: | Line 177: | ||
=Optimization= | =Optimization= | ||
The refactoring process included reducing the database calls, loops and redundant storage and computation in all classes associated with | The refactoring process included reducing the database calls, loops and redundant storage and computation in all classes associated with Lleaderboard functionality. While refactoring, the team has ensured to improve the readability of the code too by renaming ambiguous method and variable names and adding relevant comments to explain the objective of a code construct. | ||
== Code Optimization == | == Code Optimization == | ||
In the project description code complexity has been highlighted for several methods. [https://codeclimate.com/ Code Climate]<ref>[https://codeclimate.com/ Code Climate]</ref> has been used to measure the code complexity of the current repository of Expertiza. After refactoring, we used the same tool i.e. Code Climate to measure the code complexity. Following is the snapshot of code complexity of <code> Leaderboard.rb </code> from Code Climate. | In the project description code complexity has been highlighted for several methods. [https://codeclimate.com/ Code Climate]<ref>[https://codeclimate.com/ Code Climate]</ref> has been used to measure the code complexity of the current repository of Expertiza. After refactoring, we used the same tool i.e. Code Climate to measure the code complexity. Following is the snapshot of code complexity of <code style="background:#F0F0FF"> Leaderboard.rb </code> from Code Climate. | ||
{| class="wikitable" | {| class="wikitable" | ||
Line 188: | Line 200: | ||
== Database Optimization == | == Database Optimization == | ||
We computed the number of data base "SELECT" call generated the database driver by filtering the messages from the rails server. In the original code there were 625 | |||
We computed the number of data base ''"SELECT"'' call generated by the database driver by filtering the messages from the rails server's log. In the original code there were '''625''' database select access generated for the leaderboard view. In the new refactored code the number of select calls dropped to '''111'''. | |||
Shown below is the number of select calls generated per table. | Shown below is the number of select calls generated per table. | ||
Line 198: | Line 211: | ||
! Comment | ! Comment | ||
|- | |- | ||
| roles | | '''roles''' | ||
| 52 | | style="text-align: center;"|52 | ||
| 13 | | style="text-align: center;"|13 | ||
| Optimized the SQL query in leaderboard_helper.rb to replace multiple database calls. | | Optimized the SQL query in ''leaderboard_helper.rb'' to replace multiple database calls. | ||
|- | |- | ||
| users | | '''users''' | ||
| 25 | | style="text-align: center;"|25 | ||
| 25 | | style="text-align: center;"|25 | ||
| No Change | | No Change | ||
|- | |- | ||
| participants | | '''participants''' | ||
| 111 | | style="text-align: center;"|111 | ||
| 3 | | style="text-align: center;"|3 | ||
| All the participants associated with computed assignment list are calculated in a single database call and cached in memory rather than calling repetitively within loop. | | All the participants associated with computed assignment list are calculated in a single database call and cached in memory rather than calling repetitively within loop. | ||
|- | |- | ||
| assignments | | '''assignments''' | ||
| 16 | | style="text-align: center;"|16 | ||
| 5 | | style="text-align: center;"|5 | ||
| Database calls to ''Assignments'' table was reduced as result of removing it from multiple loops. Supporting data structure and caching enabled to achieve this reduction. | | Database calls to ''Assignments'' table was reduced as result of removing it from multiple loops. Supporting data structure and caching enabled to achieve this reduction. | ||
|- | |- | ||
| assignment_questionnaires | | '''assignment_questionnaires''' | ||
| 11 | | style="text-align: center;"|11 | ||
| 0 | | style="text-align: center;"|0 | ||
| Upon careful code investigation, we felt there is no need of any database calls to assignment_questionnaires table. This contains mapping of assignment id and questionnaire id. | | Upon careful code investigation, we felt there is no need of any database calls to ''assignment_questionnaires'' table. This contains mapping of ''assignment id'' and ''questionnaire id''. | ||
|- | |- | ||
| questionnaires | | '''questionnaires''' | ||
| 311 | | style="text-align: center;"|311 | ||
| 0 | | style="text-align: center;"|0 | ||
| Leaderboard reads the scores from ''ScoreCache'' table, which has ''revieweeId'' and a response object_type (''ReviewResponseMap'', ''AuthorFeedbackResponseMap'', etc.). Refactored code reuses the association of ''response object_type'' to the ''questionnaire_type'' and reduces the usage to 0. this is the most significant reduction in database calls. | | Leaderboard reads the scores from ''ScoreCache'' table, which has ''revieweeId'' and a response object_type (''ReviewResponseMap'', ''AuthorFeedbackResponseMap'', etc.). Refactored code reuses the association of ''response object_type'' to the ''questionnaire_type'' and reduces the usage to 0. this is the most significant reduction in database calls. | ||
|- | |- | ||
| score_caches | | '''score_caches''' | ||
| 22 | | style="text-align: center;"|22 | ||
| 1 | | style="text-align: center;"|1 | ||
| ScoreCache table has a field - ''revieweeId'' which can be either ''participantId'' or ''teamId''. All repetitive database calls was reduced to a single call by combining target participantId and teamId as a list of ''revieweeId''. | | ''ScoreCache'' table has a field - ''revieweeId'' which can be either ''participantId'' or ''teamId''. All repetitive database calls was reduced to a single call by combining target ''participantId'' and ''teamId'' as a list of ''revieweeId''. | ||
|- | |- | ||
| teams | | '''teams''' | ||
| 11 | | style="text-align: center;"|11 | ||
| 2 | | style="text-align: center;"|2 | ||
| Teams table was repetitively called within a loop to retrieve participation in a set of assignments. It was pulled out of the loop and modified to get the same result for all the teams for a larger set of assignments. | | Teams table was repetitively called within a loop to retrieve participation in a set of assignments. It was pulled out of the loop and modified to get the same result for all the teams for a larger set of assignments. | ||
|- | |- | ||
| teams_users | | '''teams_users''' | ||
| 52 | | style="text-align: center;"|52 | ||
| 52 | | style="text-align: center;"|52 | ||
| No change | | No change | ||
|- | |- | ||
| leaderboards | | '''leaderboards''' | ||
| 9 | | style="text-align: center;"|9 | ||
| 5 | | style="text-align: center;"|5 | ||
| Leaderboards table was called multiple times to retrieve the Label for scores of corresponding questionnaire type. e.g. ''"ReviewQuestionnaire" - "Submitted Work"''. It was reduced to fetch all such mapping and cached. | | Leaderboards table was called multiple times to retrieve the Label for scores of corresponding questionnaire type.<br />(e.g. ''"ReviewQuestionnaire" - "Submitted Work"''). It was reduced to fetch all such mapping and cached. | ||
|- | |- | ||
| courses | | '''courses''' | ||
| 5 | | style="text-align: center;"|5 | ||
| 5 | | style="text-align: center;"|5 | ||
| No change | | No change | ||
|- | |- | ||
Line 259: | Line 272: | ||
Google Chrome extension [https://chrome.google.com/webstore/detail/page-load-time/fploionmjgeclbkemipmkogoaohcdbig?hl=en Page Load Time]<ref> | Google Chrome extension [https://chrome.google.com/webstore/detail/page-load-time/fploionmjgeclbkemipmkogoaohcdbig?hl=en Page Load Time]<ref> | ||
[https://chrome.google.com/webstore/detail/page-load-time/fploionmjgeclbkemipmkogoaohcdbig?hl=en Page Load Time]</ref> gives time to load a page. | [https://chrome.google.com/webstore/detail/page-load-time/fploionmjgeclbkemipmkogoaohcdbig?hl=en Page Load Time]</ref> gives time to load a page. | ||
We tried this extension to measure performance change after refactoring. We noticed that the time to load page reduced by almost 15%. | We tried this extension to measure performance change after refactoring. We noticed that the time to load page reduced by almost '''15%'''. | ||
This table indicates time to load page in seconds | This table indicates time to load page in seconds | ||
{| class="wikitable" | {| class="wikitable" style="text-align: center;" | ||
|- | |- | ||
! Iteration | ! Iteration | ||
Line 269: | Line 282: | ||
|- | |- | ||
|1 | |'''1''' | ||
| 2.08 | | 2.08 | ||
| 1.76 | | 1.76 | ||
|- | |- | ||
|2 | |'''2''' | ||
| 2.02 | | 2.02 | ||
| 1.66 | | 1.66 | ||
|- | |- | ||
|3 | |'''3''' | ||
| 2.01 | | 2.01 | ||
| 1.76 | | 1.76 | ||
|- | |- | ||
|4 | |'''4''' | ||
| 1.97 | | 1.97 | ||
| 1.76 | | 1.76 | ||
|- | |- | ||
|5 | |'''5''' | ||
| 1.93 | | 1.93 | ||
| 1.72 | | 1.72 | ||
|- | |- | ||
|6 | |'''6''' | ||
| 1.97 | | 1.97 | ||
| 1.75 | | 1.75 | ||
|- | |- | ||
|7 | |'''7''' | ||
| 2.12 | | 2.12 | ||
| 1.74 | | 1.74 | ||
|- | |- | ||
|8 | |'''8''' | ||
| 1.99 | | 1.99 | ||
| 1.71 | | 1.71 | ||
|- | |- | ||
|9 | |'''9''' | ||
| 2.12 | | 2.12 | ||
| 1.80 | | 1.80 | ||
|- | |- | ||
|10 | |'''10''' | ||
| 2.00 | | 2.00 | ||
| 1.75 | | 1.75 |
Latest revision as of 09:35, 14 November 2014
Introduction
Project name: E1467: Expertiza - Refactoring LeaderBoard model<ref>E1467: Expertiza - Refactoring LeaderBoard model</ref>
Our project is to refactor the code in the "Leaderboard" functionality of the web application Expertiza<ref>Expertiza</ref>. The Expertiza project is a system to create reusable learning objects through peer review. The leaderboard functionality is to show top 3 individual scorers in each questionnaire type (ReviewQuestionnaire, AuthorFeedbackQuestionnaire, MetareviewQuestionnaire and TeammateReviewQuestionnaire), in each course of the logged-in user. The leaderboard also shows the current standing of the logged in user under personal achievements.
Expertiza project links | |||
Refactored Instance | http://152.46.18.78:3000/ | username= "user480" password= "password" | |
Original Instance | http://152.46.18.78:3001/ | ||
Repository link | https://github.com/sanjeevs/expertiza <ref>Forked repository</ref> | ||
Pull request | https://github.com/expertiza/expertiza/pull/444 <ref>Pull request</ref> |
Project Description
Classes involved: leaderboard.rb and associated other model and controllers classes.
1. Come up with an efficient way to search for participants based on assignment ids.
2. Refactor score hash for personal achievements. (method: extractPersonalAchievements
). Seperate out method which will rank individual personal achievements.
3. Refactor addEntryToCSHash
according to your new metric method.
4. Seperate out computation of CS entries(metric) and refactor it to be more modular and elegant.
5. Refactor getParticipantEntriesInAssignmentList
method. Its very complex (Complexity=142)
.
6. Refactor addEntryToCSHash
according to your new metric method.
Existing Functionality
Leaderboard.rb class gets all the assignments within all the courses taken by currently logged in user. It also fetches any independent assignments (not associated with any course), that the user has taken. Then, it fetches all the participants who have participated in the computed list of assignments and aggregates their scores based on the questionnaire type. Several questionnaire type is associated with a single assignment. e.g. Direwolf application has - Review Questionnaire, Author Feedback Questionnaire and Teammate Review Questionnaire.
Leaderboard model has following 3 important methods:
Sr. No. | Method Name | Comment |
---|---|---|
1 | getParticipantEntriesInAssignmentList(assignmentList)
|
This method is responsible for calculating the leaderboard of all the participants associated with assignments in given assignment list. |
2 | extractPersonalAchievements(csHash, courseIdList, userId)
|
This method is responsible for calculating personal aggregated score. It also calculates the ranking of currently logged in user against total users associated with the same set of courses as that of current user. |
3 | addEntryToCSHash(qtypeHash, qtype, userid, csEntry, courseid)
|
This method is called internally from Leaderboard.getParticipantEntriesInAssignmentList . This method aggregates score of a user grouped by course id, further grouped by questionnaire type.
|
Changes in Model
In the OSS project, we have refactored these methods along with other smaller methods in
Leaderboard.rb
, Leaderboard_helper.rb
, Leaderboard_controller.rb
and associated views files. We have also refactored ambiguous variable and method names.
We have keenly focussed in reducing the database calls, loops and redundant storage and computation. We have refactored many files related to Leaderboard implementation leading to reduction in overall complexity of the feature.
Also, there was a requirement in the project, that we should come up with an efficient way to search for participants based on assignment ids. However, upon investigation, we found that scores stored in table ScoreCache, gives us revieweeId which is either participantId or teamId. Therefore, we have a method getAssignmentMapping
, which creates a mapping of participant and team with corresponding assignment. This is very useful while computing leaderboard.
Changes made in the methods of model leaderboard.rb
Sr. No. | Method Name | Changes Made | Reason For Change |
---|---|---|---|
1 | getIndependantAssignments
|
Separate db call for each user assignment. | Single db call with an array of assignment ids. |
2 | getParticipantEntriesInCourses
|
Confusing code. Appends each entry to the end of list | Used concat to clarify that we are adding the independent and other assignments in courses. |
3 | sortHash
|
in place changes. | Returns a new deep copy of the updated hash. |
4 | extractPersonalAchievements
|
Confusing code with extra data base calls. | This method is refactored to use only 1 database call unlike several within the loop in its prior implementation. We have also removed redundant code computation and made the logic easier to understand. The overall complexity of this method is reduced from 72 to 35. |
5 | addEntryToCSHash
|
Confusing name and hard to follow logic. | The new method name is addScoreToResultantHash . As mentioned earlier, this is called internally by getParticipantEntriesInAssignmentList . The complexity of this method is reduced from 67 to 39 .
|
6 | getParticipantEntriesInAssignmentList
|
This method was the most complex method in the class. | The new refactored method name is getParticipantsScore . The method was refactored on various aspects like reducing database calls drastically, removing redundant code computation, redundant data storage in complex group of hashes and series of conditional statements. We would like to mention that previously, the method had 10 database calls within loop and 1 outside loop. After refactoring, the new method has just 1 database call within loop and 3 outside loops. Also, the overall code complexity is reduced from 142 to 64.
|
Changes in Helper
Changes made in methods of Helper leaderboard_helper.rb
Sr. No. | Method Name | Changes Made | Reason For Change |
---|---|---|---|
1 | userIsInstructor
|
Rewrote to reduce the number of database calls. | This method was refactored to reduce multiple database calls. 4 database calls was replaced by single call. |
2 | studentInWhichCourses
|
Rewrote to reduce the number of database calls. | single call. |
3 | getTop3Leaderboards
|
Dead code | Removed. |
There are several other changes in the views and controller which deal with renaming of the methods
and variables, adding proper comments etc and minor code refactoring. Please refer to our forked
github repository to view those changes.
Design Pattern
Leaderboard is a separate implementation which includes reading existing scores for the sole purpose of creating the leaders in different categories. As the task is more related to calculate and manipulate data, it was not feasible to implement any design pattern. We reduced the complexity and redundancy of database calls by reducing database calls from 625 to 111 to do the same task.
Testing
On VCL session there are 2 instances running. On port 3001
the original instance is setup and on port 3000
the refactored instance is setup. Login by any user and navigate to Home > Leaderboard to test the functionality.
In below images we have shown that the output after refactoring is same. We changed the order in which the output is displayed to make it more coherent.
Note for testing : It is recommended that in order to test the leaderboard functionality by any user who has participated in several assignments, some of them which is associated with any course and there are other existing users who have participated in the same assignment.
Expertiza Test instances links
View of Refactored Leaderboard | View of Original Leaderboard |
---|---|
|
Optimization
The refactoring process included reducing the database calls, loops and redundant storage and computation in all classes associated with Lleaderboard functionality. While refactoring, the team has ensured to improve the readability of the code too by renaming ambiguous method and variable names and adding relevant comments to explain the objective of a code construct.
Code Optimization
In the project description code complexity has been highlighted for several methods. Code Climate<ref>Code Climate</ref> has been used to measure the code complexity of the current repository of Expertiza. After refactoring, we used the same tool i.e. Code Climate to measure the code complexity. Following is the snapshot of code complexity of Leaderboard.rb
from Code Climate.
Code complexity of original leaderboard implementation | Code complexity of refactored leaderboard implementation |
---|---|
The report shows that all the 3 methods to be refactored have improved code complexity by over 50%. Following is the overall improvement report by Code Climate.
Database Optimization
We computed the number of data base "SELECT" call generated by the database driver by filtering the messages from the rails server's log. In the original code there were 625 database select access generated for the leaderboard view. In the new refactored code the number of select calls dropped to 111. Shown below is the number of select calls generated per table.
Table Name | Original Version | Refactored Version | Comment |
---|---|---|---|
roles | 52 | 13 | Optimized the SQL query in leaderboard_helper.rb to replace multiple database calls. |
users | 25 | 25 | No Change |
participants | 111 | 3 | All the participants associated with computed assignment list are calculated in a single database call and cached in memory rather than calling repetitively within loop. |
assignments | 16 | 5 | Database calls to Assignments table was reduced as result of removing it from multiple loops. Supporting data structure and caching enabled to achieve this reduction. |
assignment_questionnaires | 11 | 0 | Upon careful code investigation, we felt there is no need of any database calls to assignment_questionnaires table. This contains mapping of assignment id and questionnaire id. |
questionnaires | 311 | 0 | Leaderboard reads the scores from ScoreCache table, which has revieweeId and a response object_type (ReviewResponseMap, AuthorFeedbackResponseMap, etc.). Refactored code reuses the association of response object_type to the questionnaire_type and reduces the usage to 0. this is the most significant reduction in database calls. |
score_caches | 22 | 1 | ScoreCache table has a field - revieweeId which can be either participantId or teamId. All repetitive database calls was reduced to a single call by combining target participantId and teamId as a list of revieweeId. |
teams | 11 | 2 | Teams table was repetitively called within a loop to retrieve participation in a set of assignments. It was pulled out of the loop and modified to get the same result for all the teams for a larger set of assignments. |
teams_users | 52 | 52 | No change |
leaderboards | 9 | 5 | Leaderboards table was called multiple times to retrieve the Label for scores of corresponding questionnaire type. (e.g. "ReviewQuestionnaire" - "Submitted Work"). It was reduced to fetch all such mapping and cached. |
courses | 5 | 5 | No change |
Performance Optimization
Google Chrome extension Page Load Time<ref> Page Load Time</ref> gives time to load a page. We tried this extension to measure performance change after refactoring. We noticed that the time to load page reduced by almost 15%.
This table indicates time to load page in seconds
Iteration | Original Version | Refactored Version |
---|---|---|
1 | 2.08 | 1.76 |
2 | 2.02 | 1.66 |
3 | 2.01 | 1.76 |
4 | 1.97 | 1.76 |
5 | 1.93 | 1.72 |
6 | 1.97 | 1.75 |
7 | 2.12 | 1.74 |
8 | 1.99 | 1.71 |
9 | 2.12 | 1.80 |
10 | 2.00 | 1.75 |
Average | 2.02 | 1.74 |
Future Work
Based on use case and requirement, we can implement a metric to calculate final scores based on weights of an assignment or a questionnaire type. However, it solely depends on the the requirement whether such logic should be implemented in the Leaderboard model or the Score computation model. The team recommends that such manipulation and calculation of scores should not be part of Leaderboard model. Leaderboard model should focus on determining the eligible participants and compute the final leaderboard list.
References
<references/>