A big pile of refactoring #140
Open
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
Contributor
k-stewart
commented
May 10, 2018
•
k-stewart
commented
- Move PhantomJS logic into its own files, so we can easily swap it out with something else
- remove unused methods
- separate concerns
- split multi-concern files into single-concern files
- use class << self when there are more than one class method
- simplify a bunch of if trees and case switches
- remove some unnecessary cross-object dependencies
- DRY up duplicated logic
- use .map instead of .each when the only goal is to push things to an array
|
We're pretty behind the master branch and don't generally keep up (and I'm not a developer exactly myself), so I don't have many opinions. I'd just say please keep the documentation updated as to any changes in the front-facing API (rake tasks, etc...). |
|
Will do. |
k-stewart
force-pushed
the
refactor-congress-members
branch
from
044f92b
to
920a52f
Compare
Removes aliases that make common congress_member instance methods into class methods. I wonder why those seemed useful.
* shorten long lines * move single-use method into the only place it's used
* DRY up last_response_json
* use `let` to create instance methods
* use string interpolation instead of `'' + foo.to_s`
* break the crazy-long execute method into smaller methods * move some code that doesn't pertain to the model out of the model * fewer nested if statements * put `extends` calls and ActiveRecord methods at the top of the file * don't pass the CongressMember, the record already knows its CongressMember.
* remove unused `fill_out_form!` method * shallower if statments * move code into named methods
Split the giant factories.rb into smaller, single-model factories.
Move the list into a constant that the wider app can rely on
* Everything is a class method; use class << self instead of declaring individually * Remove unused methods
Smaller, more focused files are more legible than larger ones.
* delegate to a named scope in CongressMemberAction * use new hash syntax
Now it will be easy to swap out PhantomJS with something else! * the CongressMemberAction model doesn't need to know about Capybara, * move screenshot methods into their own file
k-stewart
force-pushed
the
refactor-congress-members
branch
from
920a52f
to
c5f15a6
Compare
Pull the logic into a tidy lib class
* code style consistency * DRY up repeated logic * keep constants in one place * more expressive names
* DRY up repeated logic * use .map instead of instantiating and pushing to an ary * fix some typos
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.