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, '-');
}​
});