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

Already on GitHub? Sign in to your account

Refactor voting handlers #99

Merged
merged 3 commits into from May 6, 2013

Conversation

Projects
None yet
4 participants
Contributor

nrk commented Nov 1, 2011

This pull request reduces code duplication in app.js and fixes a bug that was causing the inability to vote the main comment highlighted in the comment page.

Contributor

beanz commented Nov 1, 2011

Looking at the diff between handle_news_vote and handle_comment_vote, it would be quite simple to create:

function handle_vote(vote_type, item_type, id, apisecret) {

with item_type 'news or 'comment'.

Contributor

nrk commented Nov 1, 2011

@beanz you are right, I had the same idea but decided to wait to see how we should handle the update of the number of up/down votes for news and points for comments after voting, since these two actions follow two completely different logics. This could be easily implemented with an optional callback argument, if @antirez finds it useful I can commit a couple of more changes to this PR while I'm at this.

Also, I just realized that apisecret actually lives in the global scope so there's no need to have it as an argument. I'll push a fix for this later.

@nrk nrk Remove 'apisecret' from the parameters for the voting event handlers.
The apisecret variable lives in the global scope, so there is no actual need to
pass it around as an argument.
a3a05c5
Contributor

nrk commented Nov 1, 2011

In the end I couldn't resist and so this is what I came up with with some further refactoring of the code following @beanz's comment. It's on a different branch on my repository, if it looks good I can merge it into this pull request.

Owner

antirez commented Oct 9, 2012

This looks great, I need to check if this does not break existing code, and if so, merging it. Thanks.

Owner

antirez commented Oct 9, 2012

p.s. what I mean is that after 11 months I'm not sure if there are more recent changes that will break your fixes. But it looks like everything is fine actually...

Collaborator

fcambus commented May 1, 2013

I've been running the code from the 'refactor_voting_handlers2' branch in production for some time, everything works flawlessly. Could you merge it to this pull request @nrk ? Thanks!

@fcambus fcambus merged commit a3a05c5 into antirez:master May 6, 2013

Collaborator

fcambus commented May 6, 2013

Merged the 'refactor_voting_handlers2' branch from @nrk repository. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment