current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
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 have the following in script in my web app which contains a lot of duplication. How can I cut down on this duplication and generally improve my code? 

$('a#newAuthority').on("click", function() {
    var $newAuthorityContainer = $(".new-authority-container");

    var $row = $('<div />', {
        class: 'row'
    }).appendTo($newAuthorityContainer);

    var $large11 = $('<div />', {
        class: 'large-11 columns'
    }).appendTo($row);

    var $input = $('<input>', {
        type: 'text',
        placeholder: '[email protected]',
        class: 'authority-email'
    }).appendTo($large11);

    var $large1 = $('<div />', {
        class: 'large-1 columns'
    }).appendTo($row);

    var $removeContainer = $('<div />', {
        class: 'remove-container'
    }).appendTo($large1);

    var $remove = $('<a>', {
        href: '#',
        class: 'remove'
    }).html('<i class="fa fa-fa fa-remove"></i>').appendTo(
        $removeContainer);
    return false;
});
$('a#newMember').on("click", function() {
    var $newMemberContainer = $(".new-member-container");

    var $row = $('<div />', {
        class: 'row'
    }).appendTo($newMemberContainer);

    var $large11 = $('<div />', {
        class: 'large-11 columns'
    }).appendTo($row);

    var $input = $('<input>', {
        type: 'text',
        placeholder: '[email protected]',
        class: 'authority-email'
    }).appendTo($large11);

    var $large1 = $('<div />', {
        class: 'large-1 columns'
    }).appendTo($row);

    var $removeContainer = $('<div />', {
        class: 'remove-container'
    }).appendTo($large1);

    var $remove = $('<a>', {
        href: '#',
        class: 'remove'
    }).html('<i class="fa fa-fa fa-remove"></i>').appendTo(
        $removeContainer);
    return false;
});

$('input.button').on("click", function() {

    var $authorityInputs = $('.authority-email');

    var authortyEmails = [].map.call($authorityInputs, function(
        $authorityInputs) {
        return $authorityInputs.value;
    }).join(',');

    if ($('body.teamsnew').length) {
        $('input#team_authority_emails').val(authortyEmails);
    }

    if ($('body.teamsedit').length) {
        $('input#team_authority_emails').val('{' + authortyEmails + '}');
    }

    var $memberInputs = $('.member-email');

    var memberEmails = [].map.call($memberInputs, function(
        $memberInputs) {
        return $memberInputs.value;
    }).join(',');

    if ($('body.teamsnew').length) {
        $('input#team_member_emails').val(memberEmails);
    }

    if ($('body.teamsedit').length) {
        $('input#team_member_emails').val('{' + memberEmails + '}');
    }
});

$(document).on('click', '.remove', function() { 
    $(this).closest('.row').remove();
    return false;
});
share|improve this question
    
Please see What you may and may not do after receiving answers –  rolfl Jun 19 at 12:04

2 Answers 2

up vote 1 down vote accepted

Extract the code that creates a row to a function, make it return the row created with all the elements inside of it, so that you can use it like this:

var $newAuthorityContainer = $(".new-authority-container");
var $row = createRow().appendTo($newAuthorityContainer);

And then again later:

var $newMemberContainer = $(".new-member-container");
var $row = createRow().appendTo($newMemberContainer);

Alternatively, pass in the container that you want to add the row to, for example:

var $newMemberContainer = $(".new-member-container");
createRow($newMemberContainer);

The function for this latter example:

function createRow($container) {
    var $row = $('<div />', {
        class: 'row'
    }).appendTo($container);

    var $large11 = $('<div />', {
        class: 'large-11 columns'
    }).appendTo($row);

    var $input = $('<input>', {
        type: 'text',
        placeholder: '[email protected]',
        class: 'authority-email'
    }).appendTo($large11);

    var $large1 = $('<div />', {
        class: 'large-1 columns'
    }).appendTo($row);

    var $removeContainer = $('<div />', {
        class: 'remove-container'
    }).appendTo($large1);

    var $remove = $('<a>', {
        href: '#',
        class: 'remove'
    }).html('<i class="fa fa-fa fa-remove"></i>').appendTo(
        $removeContainer);
}

I don't see other troubling duplication.

However, I see a bit troubling typo in the variable name authortyEmails. This could lead to errors, so I suggest to fix that.

share|improve this answer
    
Thanks @janos. Can you see any other improvements, gotchas that I could use to improve this script? –  Thomas Taylor Jun 17 at 6:24
    
I just found a semantic error in my code. The problem with your solution (now the code has changed) is that the var $input class name changes. Ideally I'd like to be able to DRY out the HTML creation and append the layout with the correct class to the correct container. –  Thomas Taylor Jun 17 at 6:41
up vote 1 down vote

General advice on code duplication is to detect pieces of duplicated code and extract them into separate, reusable, functions.

Example:

 var $row = $('<div />', {
    class: 'row'
}).appendTo($newMemberContainer);

Create row logic can be extraced:

function createRow() {
  return $('<div />', {
    class: 'row'
  });
}

and the whole piece of code would look like:

createRow().appendTo($newMemberContainer);
createRow().appendTo($newAuthorityContainer);

Sometimes you'll probably need to parametrize the functions. In your case if you have larger blocks, you can create functions for creating whole components.

And a bit off topic maybe, but if you do a lot of HTML creation / modification in your JS, you probably should probably consider switching to knockout / angularJS, which is much better than pure jQuery manipulations.

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.