Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I'm finally "done" with my small prototype for a element inside my website. I'm just a learner in the jQuery code and that reflects in my code. It's bloated, a lot of functions do the same thing but for a red or green button which results in this kind of code

$('.submenu').on('click','.rood',function() {
    $(this).closest('.container_vragen').fadeOut(400, function() {
        $(this).closest('.container_vragen').css({overflow : 'hidden', color : 'red'});

$('.submenu').on('click','.groen',function() {
    $(this).closest('.container_vragen').fadeOut(400, function() {
        $(this).closest('.container_vragen').css({overflow : 'hidden', color : 'green'});

As you can see most of the functions are the same, only the color and the .appedTo id changes. So what's the best way to merge this code? I'm thinking there needs to be an if statement which filters what button is clicked. But I can't figure it out on my own.

share|improve this question

You should think about chaining, to avoid repeating some class names :

function doAnimation($element, $destElement, animationColor) {
    var $container_vragen = $element.closest('.container_vragen');
    $container_vragen.fadeOut(400, function() {
                overflow : 'hidden',
                color : animationColor

var $niet_geregeld = $('#niet_geregeld');
var $geregeld = $('#geregeld');

.on('click','.rood',function() {
    doAnimation($(this), $niet_geregeld,'red');
.on('click','.groen',function() {
    doAnimation($(this), $geregeld,'green');

I think preselecting #geregeld and #niet_geregeld is better for performances.

You should rename the doAnimation function to its real purpose, like animateSorting or animateForThatPurpose

share|improve this answer

Store your options separately, then loop over them:

var $submenu = $('.submenu');
var map = {
    'rood': {
        color: 'red',
        appendTo: 'niet_geregeld'
    'groen': {
        color: 'green',
        appendTo: 'geregeld'

$.each(map, function (elClass, options) {
    $submenu.on('click', '.' + elClass, function () {

        var $container = $(this).closest('.container_vragen');

        $container.fadeOut(400, function() {
                .css({overflow : 'hidden', color : options.color})
                .appendTo("#" + options.appendTo)

share|improve this answer

Make a function.

function doStuff(obj, c) {
  obj.css({overflow: 'hidden', color: c}).fadeIn(400);

Then call it

$('#submenu').click(function () {
  doStuff($('#geregeld'), 'green');

(simplified example)

Feel free to mess around:

share|improve this answer
I understand what you're trying to do, but it's not working. If I use this code the click action on .groen is not calling the doStuff function. – Peter Boomsma Jan 10 '13 at 18:50
@PeterBoomsma : updated :) – user1882585 Jan 10 '13 at 19:50

You can combine the two like this

$('.submenu').on('click', '.groen,.rood', function () {
  var $el = $(this);
  var red = $'.rood'); // is this element .rood
    overflow: 'hidden',
    color: red ? 'red' : 'green' // if it's .rood make it red.. else make green
    .appendTo(red ? "#niet_geregeld" : "#geregeld") // if .rood appendto #niet_geregeld else append to #geregeld

and use chaining so you don't have to keep querying the dom for the element.

share|improve this answer

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.