patternjavascriptMinor
Simplifying up/down vote code
Viewed 0 times
codevotedownsimplifying
Problem
I've got this working JavaScript (example is [here][1]), which should work the same as on the Stack Exchange network.
How can I simplify it a bit?
```
function yellow() {
return 'rgb(255, 255, 0)';
}
$(function() {
$(".post-upvote").click(function() {
// ajax(url + "upvote/" + $(this).attr('data-postid'), false, false);
if ($(this).parent().children('.post-downvote').css('background-color') == yellow()) { // user upvoted so let's delete upvote
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) + parseInt(1));
}
$(this).parent().children('.post-downvote').css('background-color', 'white');
if ($(this).css('background-color') == yellow()) { // if it's yellow, user is canceling his downvote
$(this).css('background-color', 'white');
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) - parseInt(1));
}
else {
$(this).css('background-color', 'yellow');
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) + parseInt(1));
}
});
$(".post-downvote").click(function() {
// ajax(url + "downvote/" + $(this).attr('data-postid'), false, false);
if ($(this).parent().children('.post-upvote').css('background-color') == yellow()) { // user upvoted so let's delete upvote
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) - parseInt(1));
}
$(this).parent().children('.post-upvote').css('background-color', 'white');
if ($(this).css('background-color') == yellow()) { // if it's yellow, user is canceling his downvote
$(this).css('background-color', 'white');
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) + parseI
How can I simplify it a bit?
```
function yellow() {
return 'rgb(255, 255, 0)';
}
$(function() {
$(".post-upvote").click(function() {
// ajax(url + "upvote/" + $(this).attr('data-postid'), false, false);
if ($(this).parent().children('.post-downvote').css('background-color') == yellow()) { // user upvoted so let's delete upvote
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) + parseInt(1));
}
$(this).parent().children('.post-downvote').css('background-color', 'white');
if ($(this).css('background-color') == yellow()) { // if it's yellow, user is canceling his downvote
$(this).css('background-color', 'white');
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) - parseInt(1));
}
else {
$(this).css('background-color', 'yellow');
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) + parseInt(1));
}
});
$(".post-downvote").click(function() {
// ajax(url + "downvote/" + $(this).attr('data-postid'), false, false);
if ($(this).parent().children('.post-upvote').css('background-color') == yellow()) { // user upvoted so let's delete upvote
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) - parseInt(1));
}
$(this).parent().children('.post-upvote').css('background-color', 'white');
if ($(this).css('background-color') == yellow()) { // if it's yellow, user is canceling his downvote
$(this).css('background-color', 'white');
$(this).parent().children('.post-votes').text(parseInt($(this).parent().children('.post-votes').text()) + parseI
Solution
Get rid of antipatterns like
Just say
Factor out common code like
and
into variables.
Combine the two handlers thus
parseInt(1)Just say
1 instead.Factor out common code like
$(this).parent().children('.post-votes')and
$(this).css('background-color')into variables.
Combine the two handlers thus
function vote(isUpvote) {
var control = $(this);
var otherControl = control.parent().children(
isUpvote ? ".post-downvote" : ".post-upvote");
var postVotes = control.parent().children(".post-votes");
var ajaxHandler = isUpvote ? "upvote/" : "downvote/";
// ajax(url + ajaxHandler + control.attr('data-postid'), false, false);
// user voted so let's delete the other control
if (otherControl.css("background-color") == yellow()) {
postVotes.text(+(postVotes.text()) + 1);
}
control.parent().children(otherId).css("background-color", "white");
// if it's yellow, user is cancelling their vote
if (control.css("background-color") == yellow()) {
control.css("background-color", "white");
postVotes.text(+(postVotes.text()) - 1);
} else {
control.css("background-color", "yellow");
postVotes.text(+(postVotes.text()) + 1);
}
}
$(".post-upvote" ).click(function() { vote.call(this, true); });
$(".post-downvote").click(function() { vote.call(this, false); });Code Snippets
parseInt(1)$(this).parent().children('.post-votes')$(this).css('background-color')function vote(isUpvote) {
var control = $(this);
var otherControl = control.parent().children(
isUpvote ? ".post-downvote" : ".post-upvote");
var postVotes = control.parent().children(".post-votes");
var ajaxHandler = isUpvote ? "upvote/" : "downvote/";
// ajax(url + ajaxHandler + control.attr('data-postid'), false, false);
// user voted so let's delete the other control
if (otherControl.css("background-color") == yellow()) {
postVotes.text(+(postVotes.text()) + 1);
}
control.parent().children(otherId).css("background-color", "white");
// if it's yellow, user is cancelling their vote
if (control.css("background-color") == yellow()) {
control.css("background-color", "white");
postVotes.text(+(postVotes.text()) - 1);
} else {
control.css("background-color", "yellow");
postVotes.text(+(postVotes.text()) + 1);
}
}
$(".post-upvote" ).click(function() { vote.call(this, true); });
$(".post-downvote").click(function() { vote.call(this, false); });Context
StackExchange Code Review Q#4312, answer score: 6
Revisions (0)
No revisions yet.