Tidying up jquery code for linking inputs

Question

I have three input boxes that are all linked to each other by some pretty basic rules:

• Editing the page title will copy the contents to menu title
  • Unless menu title has been manually edited
  • clearing menu title sets it back to automatic
• Editing the menu title will copy the contents as a url slug to slug
  • Unless the slug has been manually edited
  • clearing the slug sets it back to automatic
  • Editing page title counts as editing menu title if it is set to automatic
• The Page Title, and the other two boxes are never visible at the same time!

​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​

Here is a demo: http://jsfiddle.net/6VPpj/1/

and here is my rather horrible code, how can I clean this up?

$(function(){
var automatic = true;
var pager = true;
var cache = {};
var slug = $('#slug');
var menu = $('#menu_title');
var page = $('#page_title');
page.on('change', function() {
 if (pager) {
 menu.val(page.val());
 menu.keyup();
 }
});
menu.on('keypress keyup keydown change', function() {
 if (automatic) {
 var n = menu.val();
 if (n in cache) {
 slug.val(cache[n]);
 return;
 }
 cache[n] = toSlug(n);
 slug.val(cache[n]);
 return;
 }
});
menu.on('change', function() {
 if (menu.val() == '') {
 pager = true;
 page.change();
 }
 else {
 pager = false;
 }
});
slug.on('change', function() {
 if (slug.val() == '') {
 automatic = true;
 menu.keyup();
 }
 else {
 automatic = false;
 var n = slug.val();
 if (n in cache) {
 slug.val(cache[n]);
 return;
 }
 cache[n] = toSlug(n);
 slug.val(cache[n]);
 return;
 }
});
function toSlug(Text) {
 return Text.toLowerCase().replace(/ /g, '-').replace(/[^\w-]+/g, '').replace(/--+/g, '-');
}​
});

Answer 1

It's not so horrible. You managed to cache your jQuery selectors, which is more than a lot of people can say. Good job there.

---

automatic and pager are not super great names in this context. I suggest renaming them to autoSlug/autoMenu or manualSlug/manualMenu.

---

You are caching toSlug, but really, it's not complex enough to warrant caching. I did some quick and dirty testing and found that you save about 10ms per 1,000 executions. That is not worth anything here. So, to reduce code size and bug potential, I suggest removing all caching.

---

Variables and parameters should not be capitalized unless they represent a class. As such, the Text parameter in toSlug should be text.

---

Finally, and this is more of a personal preference, but I find that code is easier to read and follow when reading functions and not event handlers. By that I mean this: instead of calling menu.change(), why not move that code to a function and call updateMenu()?

Here is the final result:

$(function(){
var autoSlug = true;
var autoMenu = true;
var slug = $('#slug');
var menu = $('#menu_title');
var page = $('#page_title');
page.on('change', function() {
 if (autoMenu) {
 updateMenu()
 }
});
menu.on('change', function() {
 autoMenu = (menu.val() == '');
 updateMenu()
});
slug.on('change', function() {
 autoSlug = (slug.val() == '');
 updateSlug();
});
function updateMenu(){
 if(autoMenu){
 menu.val(page.val())
 }
 if(autoSlug){
 updateSlug()
 }
}
function updateSlug(){
 var text = autoSlug ? menu.val() : slug.val();
 slug.val(toSlug(text));
}
function toSlug(text) {
 return text.toLowerCase().replace(/ /g, '-').replace(/[^\w-]+/g, '').replace(/--+/g, '-');
}​
});