Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

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'});
        $(this).closest('.container_vragen').appendTo("#niet_geregeld");
        $(this).closest('.container_vragen').fadeIn(400);
    });
});

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

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

4 Answers

up vote 1 down vote

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() {
        $container_vragen.css({
                overflow : 'hidden',
                color : animationColor
            })
            .appendTo($destElement)
            .fadeIn(400);
    });
}

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

$('.submenu')
.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
up vote 1 down vote

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() {
            $container
                .css({overflow : 'hidden', color : options.color})
                .appendTo("#" + options.appendTo)
                .fadeIn(400);
        });

    });
});
share|improve this answer
up vote 1 down vote

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: http://jsfiddle.net/rQFHt/

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 at 18:50
@PeterBoomsma : updated :) – user1882585 Jan 10 at 19:50
up vote 1 down vote

You can combine the two like this

$('.submenu').on('click', '.groen,.rood', function () {
  var $el = $(this);
  var red = $el.is('.rood'); // is this element .rood
  $el.closest('.container_vragen').css({
    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
    .fadeIn(400)
});

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

share|improve this answer

Your Answer

 
discard

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.