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?
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?
-
2
Don't repeat yourself.
$(active)is creating a jQuery object over and over again. Create it once, store it in a variable. ;) – epascarello Commented May 31, 2013 at 13:11 - 1 The problem es down to maintainability, both engineering and software practice: en.wikipedia/wiki/Maintainability and en.wikipedia/wiki/Software_maintenance. Depending on context, this is a enormous, or very small issue. It is still an issue. – Automatico Commented May 31, 2013 at 13:18
- @epascarello actually in this case, active is actually changing between the the object creations – Ricardo Rodrigues Commented May 31, 2013 at 13:31
- Thanks everyone who mented 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 Commented May 31, 2013 at 13:46
-
1
@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 Commented May 31, 2013 at 15:22
6 Answers
Reset to default 9You 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);
}
}
"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.
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');});
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.
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>
You could write something like this, fully extensible:
(only defined active,disabled variables so this actually works in jsfiddle)
jsfiddle: http://jsfiddle/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);
}
}