Javascript/Jquery - Don't repeat yourself

Question

I've seen this so many times on the internet. People say to don't repeat yourself in programming languages. I wrote this script for my webpage but I repeated myself quite a bit.

Is it really that big of a deal?

Should I make a function?

How do I do it?

var active = '.teachers';
var disabled = '.teacher-link';
var width = $('.teachers .staff-outer-container').children().size() * 180;
$('.staff-outer-container').css('width', width + 'px');

$('.teacher-link').click(function() {
    if (active != '.teachers') {
        $(active).hide();
        active = '.teachers';
        $(active).show();
        width = $('.teachers .staff-outer-container').children().size() * 180;
        $('.teachers .staff-outer-container').css('width', width + 'px');
        $(disabled).removeClass('active').addClass('clickable');
        disabled = this;
        $(disabled).removeClass('clickable').addClass('active');
        $('#type').text('Teachers');
    }
});
$('.admin-link').click(function() {
    if (active != '.administrators') {
        $(active).hide();
        active = '.administrators';
        $(active).show();
        width = $('.administrators .staff-outer-container').children().size() * 180;
        $('.administrators .staff-outer-container').css('width', width + 'px');
        $(disabled).removeClass('active').addClass('clickable');
        disabled = this;
        $(disabled).removeClass('clickable').addClass('active');
        $('#type').text('Administrators');
    }
});
$('.support-link').click(function() {
    if (active != '.support') {
        $(active).hide();
        active = '.support';
        $(active).show();
        width = $('.support .staff-outer-container').children().size() * 180;
        $('.support .staff-outer-container').css('width', width + 'px');
        $(disabled).removeClass('active').addClass('clickable');
        disabled = this;
        $(disabled).removeClass('clickable').addClass('active');
        $('#type').text('Support Staff');
    }
});

edit Thanks for everyone's input! I'm confused on how to implement these functions. This is what I got: $('.teacher-link').click(handle_click('.teachers', 'Teachers')); I tried this out and it didn't work. Also where do I place the function? Do I place it inside or outside $(document).ready ? Is it best to put functions at the start or the end of my script?

Don't repeat yourself. `$(active)` is creating a jQuery object over and over again. Create it once, store it in a variable. ;) — epascarello, May 31 '13 at 13:11
The problem comes down to maintainability, both engineering and software practice: http://en.wikipedia.org/wiki/Maintainability and http://en.wikipedia.org/wiki/Software_maintenance. Depending on context, this is a enormous, or very small issue. It is still an issue. — Automatico, May 31 '13 at 13:18
@epascarello actually in this case, active is actually changing between the the object creations — Ricardo Rodrigues, May 31 '13 at 13:31
Thanks everyone who commented and answered! I don't work with functions so I have a couple questions. Do i place the function inside $(document).ready? Or is the function outside of it? Where is the best place to put functions? At the start or end of my script? — Brett Merrifield, May 31 '13 at 13:46
@BrettMerrifield The function should go outside of `$(document).ready`, and I don't think it matters if its before or after. I am a little rusty on my js, so if someone can correct me, that would be great. — Alex Gittemeier, May 31 '13 at 15:22

score 9 · Accepted Answer · answered May 31 '13 at 13:04

You could build a function like this, and call it like:

handle_click('.teachers', 'Teachers');
handle_click('.adminstrators', 'Administrators');
etc.

Function:

function handle_click(target, target_text) {
    if (active != target) {
        $(active).hide();
        active = target;
        $(active).show();
        width = $(target + ' .staff-outer-container').children().size() * 180;
        $(target + ' .staff-outer-container').css('width', width + 'px');
        $(disabled).removeClass('active').addClass('clickable');
        disabled = this;
        $(disabled).removeClass('clickable').addClass('active');
        $('#type').text(target_text);
    }
}

score 5 · Answer 2 · edited May 31 '13 at 14:00

"Is it such a big deal"?

It is NOT a big deal as long as everything works ok.
It is a big deal if something breaks down or you have to change/add some features.
It is NOT a big deal if it's a small piece of code which you handle all by yourself
It is a big deal if someone else needs to work with your code and it will be extended in the future.

Personaly I think that if you are hours after deadline and stuff needs to be published ASAP, you can keep messy and redundant code - but only if it is refactored afterwards. The problem is that, probably, once the code is published no one will look and refactor it, unless it get's broken or generates bugs.

Look at your code - what if someone decides that a new feature would be generating dynamic objects and handling them with those functions, using loops and stuff? With your code this is impossible and after all you'll need to make it automatic. So why not make it correct in the first place? I think that making the code automatic will cost muuuuuch less time than fixing it if something bad happens.

zmo · Answer 3 · 2013-05-31T13:13:34.217

That's one of the basics of sotware engineering. If a task has to be made twice, it has to be automatized. There you got a pattern that can be put in a function, or a class method, depending on the context of usage. I can give you an example of such a function, but that wouldn't be a good example, without having a thorough thought of that in the execution context.

function link_click(linkClass, targetClass, text) {
    $(linkClass).click(function() {
        if (active != targetClass) {
            $(active).hide();
            active = targetClass;
            $(active).show();
            width = $(linkClass + ' .staff-outer-container').children().size() * 180;
            $(linkClass + ' .staff-outer-container').css('width', width + 'px');
            $(disabled).removeClass('active').addClass('clickable');
            disabled = this;
            $(disabled).removeClass('clickable').addClass('active');
            $('#type').text(text);
        }
});

There are so many different ways to do it (as you can see in the different answers over here), and I'm pretty sure none of them is totally a fit for your usage. You may prefer to make a class that has one or more methods for that purpose, so you can make different ways to execute your function, or create a new event or something else I don't think of now...

Take a break, look overall at your code, what you would need for potential future improvements so the stuff you write today don't get in the way tomorrow.

score 2 · Answer 4 · answered May 31 '13 at 13:05

function switchActive(tag,name)
{
    if (active != tag) {
        $(active).hide();
        active = tag;
        $(active).show();
        width = $(tag+' .staff-outer-container').children().size() * 180;
        $(tag+' .staff-outer-container').css('width', width + 'px');
        $(disabled).removeClass('active').addClass('clickable');
        disabled = this;
        $(disabled).removeClass('clickable').addClass('active');
        $('#type').text(name);
    }
}
$('.teacher-link').click(function(){switchActive('.teacher','Teachers');});
$('.admin-link').click(function(){switchActive('.administrators','Administrators');});
$('.suppport-link').click(function(){switchActive('.support','Support Staff');});

score 1 · Answer 5 · answered May 31 '13 at 13:30

You are doing well. However you can improve that your code be more productivity and reusability. It is not good solution but it will help you for some idea.

<script>

    $(function(){

        $('.tab-link').click(function() {
            if($(this).hasClass("active") == false) {

                var target = $( $(this).attr("href") );

                $(".tab-section").hide();
                target.show();

                $(".tab-link").removeClass("active").addClass("clickable");
                $(this).removeClass("clickable").addClass("active");

                var container = target.find("staff-outer-container");
                var width = container.children().size() * 180;
                container.css("width", width + "px");

                $("#type").text($(this).html());
            }
        });
    });
</script>

<a class="tab-link" href="#staff-teacher">Teachers</a>
<a class="tab-link" href="#staff-admin">Administrators</a>
<a class="tab-link" href="#staff-support">Support Staff</a>

<h1 id="type">Teachers</h1>

<div id="staff-teacher" class="tab-section">
    <div class="staff-outer-container">
        #Teacher List
    </div>
</div>
<div id="staff-admin" class="tab-section">
    <div class="staff-outer-container">
        #Admin List
    </div>
</div>
<div id="staff-support" class="tab-section">
    <div class="staff-outer-container">
        #Staff List
    </div>
</div>

Reading this code makes so much sense to me, Its so much cleaner than mine. I need to start coding like this. Thanks +1 — Brett Merrifield, May 31 '13 at 14:46

Ricardo Rodrigues · Answer 6 · 2013-05-31T13:28:00.297

You could write something like this, fully extensible:

(only defined active,disabled variables so this actually works in jsfiddle)

jsfiddle: http://jsfiddle.net/8RaXv/2/

var active=document.getElementById('active'),
    disabled=document.getElementById('disabled'),
    teacher = {
        $link: $('.teacher-link'),
        selector: '.teachers',
        text: 'Teachers'
    },admin = {
        $link: $('.admin-link'),
        selector: '.administrators',
        text: 'Administrators'
    },support = {
        $link: $('.support-link'),
        selector: '.support',
        text: 'Support Staff'
    };

bindHandler([teacher, admin, support]);

//bind handlers
function bindHandler(items) {
    for (var i in items) {
        items[i].$link.on('click', items[i], clickHandler);
    }
}

//generic handler
function clickHandler(event) {
    var context = event.data;
    if (active !== context.selector) {
        $(active).hide();
        active = context.selector;
        $(active).show();
        var $container = $(context.selector + ' .staff-outer-container');
        width = $container.children().size() * 180;
        $container.css('width', width + 'px');
        $(disabled).removeClass('active').addClass('clickable');
        disabled = this;
        $(disabled).removeClass('clickable').addClass('active');
        $('#type').text(context.text);
    }    
}

Javascript/Jquery - Don't repeat yourself

6 Answers6

Linked