Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E2220: Refactoring reputation_web_service_controller #2291

Merged
merged 60 commits into from Apr 11, 2022

Conversation

krishnasaurabh
Copy link
Contributor

@krishnasaurabh krishnasaurabh commented Mar 8, 2022

Changes that are included in the Pull Request are:

  • Removed Unused code and commented code
  • split db_query method to (calculate_peer_review_grade, get_peer_reviews)
  • used assignment_id_list as the parameter, to reduce unused parameters
  • added meaningful method name to db_query
  • Split the json_generator into three new methods (generate_json_body, generate_json_for_peer_reviews, generate_json_for_quiz_scores)
  • Created a helper function to convert assignment_id, another_assignment_id to assignment_id_list
  • Change in number of parameters for db_query, db_query_with_quiz_score
  • added code comments for all the methods
  • split send_post_request method to conform to single responsibility principle
  • changed assignment from class variables to instance variables by using flash

A complete list of changes and description is present at https://expertiza.csc.ncsu.edu/index.php/CSC/ECE_517_Spring_2022_-_E2220:_Refactor_reputation_web_service_controller

@expertiza-bot
Copy link
Contributor

expertiza-bot commented Mar 8, 2022

1 Message
💬 Thanks for the pull request, and welcome! 🎉 The Expertiza team is excited to review your changes, and you should hear from us soon.

Please make sure the PR passes all checks and you have run rubocop -a to autocorrect issues before requesting a review.

This repository is being automatically checked for code-quality issues using Code Climate.
You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a pull request is considered ready to review.

Also, please spend some time looking at the instructions at the top of your course project writeup.
If you have any questions, please send email to expertiza-support@lists.ncsu.edu.

UUID 1 Warning
e222 ⚠️ Your pull request is more than 500 LoC.
Please make sure you did not commit unnecessary changes, such as schema.rb, node_modules, change logs.
61df Your pull request is less than 50 LoC.
If you are finished refactoring the code, please consider writing corresponding tests.
ecaa There are code changes, but no corresponding tests.
Please include tests if this PR introduces any modifications in behavior.

Generated by expertiza-bot

@coveralls
Copy link

Coverage Status

Coverage decreased (-31.5%) to 16.759% when pulling 673fe20 on krishnasaurabh:beta into 33b5c64 on expertiza:beta.

@coveralls
Copy link

coveralls commented Mar 8, 2022

Coverage Status

Coverage decreased (-19.9%) to 51.285% when pulling ef9760f on krishnasaurabh:beta into 9345f36 on expertiza:beta.

@coveralls
Copy link

Coverage Status

Coverage decreased (-31.5%) to 16.759% when pulling 673fe20 on krishnasaurabh:beta into 33b5c64 on expertiza:beta.

@expertiza-travisci-bot
Copy link

Hey @krishnasaurabh,
Your changes look good to me! 🎉

#
# result = ActiveRecord::Base.connection.select_all(query)
def db_query(assignment_id, round_num, has_topic, another_assignment_id = 0)
def calculate_peer_review_grade(response)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, I can see this method does 2 things,

  1. Getting answers based on response_id
  2. Calculating overall score
    Can you send the answers object in the method parameter?

teams.each { |team| team_ids << team.id }
def get_ids_list(tables)
id_list = []
tables.each { |table| id_list << table.id }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use map function for this?

assignment_id_list << assignment_id_two unless assignment_id_two.zero?
assignment_id_list
end

def send_post_request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is scope of improvement in this method,

  1. The URL is hardcoded. Can you find a better place to place this URL in a constant and use it here?
  2. This method should only hit the post request, anything else (like generating request body, headers, decrypting response, etc) should not be the scope of this method.

@johnbumgardner johnbumgardner merged commit c8a819b into expertiza:beta Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants