3
\$\begingroup\$

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

1 Answer 1

2
\$\begingroup\$

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.