Code Cleanup #369
Code Cleanup #369
Conversation
|
I've been trying to rebase this off the main repo's master branch, but there's an incredibly annoying problem where you have both a [Kim]: Steve, thank you for trouble shooting this issue, which I've had as well but did not know the "why". the good news is that I'm working on a branch in which jQuery is eliminated in favor of just native Angular's built-in jQuery lite and should fix the issue. @ThePletch This issue should now be fixed with pr #373. |
|
Wow, Steve. Very interesting. |
| # def self_and_descendants # dupe of all_related_proposals | ||
| # [self.root, self.root.descendants].flatten | ||
| # end | ||
|
|
||
| def related_proposals(related_sort_by = 'Most Votes') |
There was a problem hiding this comment.
I'm not a fan of this method, mostly because switching the sorting function off a string like this is a bad code smell. These functions are similar enough that this method could be heavily simplified or abstracted. In-place sorting should also be avoided if possible, especially in a situation like this where replacing sort! with sort wouldn't change the function.
There was a problem hiding this comment.
Again, no worries if you wanted to change the API here. If you did, note that the UI is currently broken here per issue #311. Nothing serious, I think just a seam library that changed something.
|
Also, could you go over your data model a little more? I'm not clear on why the proposals are organized in a full tree structure. All the tree traversal is preventing you from doing a lot of data manipulation on the database side, which would be miles faster. |
| if response.comment is null | ||
| responseMessage = "Your vote was recorded without a comment" | ||
| else | ||
| responseMessage = "Your vote was created with the comment: ""#{response.comment}""" |
There was a problem hiding this comment.
You will need to escape those quotes, \"#{response.comment}\"
|
The model (application) uses trees of related proposals, where related means forked from each other. We care about these trees because:
|
ThePletch
force-pushed
the
367-code-cleanup
branch
from
2922120
to
093c5cb
Compare
|
Would you object to removing commented-out code, since you can just bring back old code from the git history if necessary? |
…ore unneeded lines from code
|
I think that about finishes things for me unless a spec fails or you'd like me to clean up commented-out/nulled-out code (there was a line of code that was |
|
Awesome, Steve. Let me take a look this weekend! /k On Fri, Jul 3, 2015 at 1:20 PM, Steve Pletcher notifications@github.com
|
| rescue ArgumentError | ||
| pp "WARNING: Could not use google service to find hubs" | ||
| rescue ArgumentError => e | ||
| Rails.logger.error "WARNING: Could not use google service to find hubs. Error: #{e.message}" |
There was a problem hiding this comment.
This should probably be Rails.logger.warn now that I think about it.
|
@ThePletch, was this branch testing and working for you? Some of the changes, e.g. in application controller, seem to delete necessary things. As is, I'm not getting the branch to actually spin up: |
|
Commits contained in PR #381, where merge conflicts have been solved and code comments have been deleted. |
Code Clean Up Reviewed, Ruby comments removed, Rebased & Resolved Merge Conflicts for PR #369
|
Hi @ThePletch, any thoughts on this error?
That GL-CjQu .... hub ID is part of the Google Places feature, so a normal part of the API. |
Work in progress - cleaning things up, using ActiveRecord query methods over array manipulation, moving complex logic out to helper methods when possible.