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

E1770. [TFD] refactor_assignment_participant.rb #1049

Merged
merged 52 commits into from
Nov 11, 2017
Merged

E1770. [TFD] refactor_assignment_participant.rb #1049

merged 52 commits into from
Nov 11, 2017

Conversation

terryliu1995
Copy link
Contributor

first try

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.6%) to 44.21% when pulling ec9f754 on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-5.7%) to 45.193% when pulling ec9f754 on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-17.09%) to 33.766% when pulling 3ff741f on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 51.787% when pulling 4148ebf on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 51.753% when pulling 6e7a6bb on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 51.753% when pulling 404f0e9 on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 51.764% when pulling 2b0b410 on terryliu1995:master into a26cf3a on expertiza:master.

@Winbobob Winbobob self-assigned this Oct 26, 2017
@Winbobob Winbobob changed the title 1770TFD_refactor_assignment_participant.rb 1770. [TFD] refactor_assignment_participant.rb Oct 26, 2017
@Winbobob Winbobob changed the title 1770. [TFD] refactor_assignment_participant.rb E1770. [TFD] refactor_assignment_participant.rb Oct 26, 2017
@@ -1,6 +1,6 @@
require 'uri'
require 'yaml'

require 'TFD1770_refactor'
Copy link
Member

Choose a reason for hiding this comment

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

According to this configuration, maybe you do not need to require lib file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that I have to add "require". If I delete "require", the test cannot be passed.
image

end

# methods extracted from scores method:assignment_questionnaires, merge_scores, topic_total_scores, caculate_scores
def assignment_questionnaires(questions, scores)
Copy link
Member

Choose a reason for hiding this comment

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

It will be better the method name is a verb or an action, instead of a noun.

caculate_scores(scores)
end

# methods extracted from scores method:assignment_questionnaires, merge_scores, topic_total_scores, caculate_scores
Copy link
Member

Choose a reason for hiding this comment

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

Good comment! You could move this comment before the definition of scores method.

attributes = ImportFileHelper.define_attributes(row)
user = ImportFileHelper.create_new_user(attributes, session)
end
user = AssignmentParticipant.check_info_and_create(row, _row_header = nil, session)
Copy link
Member

Choose a reason for hiding this comment

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

Good refactoring!

@@ -0,0 +1,28 @@
module Instance_method
Copy link
Member

Choose a reason for hiding this comment

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

  • It will be better if you rename this file to a reasonable name, instead of TFD1770_refactor.rb.
  • Also, you may need to put these two modules into two lib files since they are quite different.
  • Also rename the module names to reasonable names, instead of Instance_method and Class_method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix them ASAP, Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have finished all the modifications except deleting "require", because if I delete it, the test will not be passed. Can you explain this why I cannot delete it?

@Winbobob Winbobob added the Merge label Oct 29, 2017
@Winbobob
Copy link
Member

Winbobob commented Oct 29, 2017

Hi team,

  • I think you did a really good job!
  • You may need to do some modifications based on my comments.

Thanks,
Zhewei

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 51.204% when pulling 7164ba3 on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 51.215% when pulling 053c27a on terryliu1995:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 51.204% when pulling 053c27a on terryliu1995:master into a26cf3a on expertiza:master.

@Winbobob Winbobob merged commit 053c27a into expertiza:master Nov 11, 2017
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

5 participants