search

antirez / lamernews

Lamer News -- an HN style social news site written in Ruby/Sinatra/Redis/JQuery
http://lamernews.com
Other
1.35k stars 200 forks source link

Refactor voting handlers #99

Closed nrk closed 11 years ago

nrk commented 13 years ago

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.

beanz commented 13 years ago

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'.

nrk commented 13 years ago

@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 commented 13 years ago

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.

antirez commented 12 years ago

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

antirez commented 12 years ago

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...

fcambus commented 11 years ago

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 commented 11 years ago

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