1
\$\begingroup\$

I've built a small slug generator that will take an input from a user and replace all of the spaces with dashes to create a nice looking slug. I'd appreciate any tips to make my code better.

<input id="nameValue" type="text"><br>
<input id="slugValue" type="text">

<script>

let nameValue = document.getElementById("nameValue");

nameValue.onkeyup = function(){
    let slugValue = document.getElementById("slugValue");

    key = event.keyCode;

    // Dash
    if(key == '189') {
        key = '0x002D';
        key_str = String.fromCharCode(key);
        slugValue.value += key_str.toLowerCase();
    }
    // Space
    if(key == '32') {
        key = '0x002D';
        key_str = String.fromCharCode(key);
        slugValue.value += key_str.toLowerCase();
    }
    // Shift
    if(key == '16') {
        key_str = null;
    }
    //Backspace
    if(key == '8') {
        key = '0x0008';
        newslug = nameValue.value.toLowerCase();
        slugValue.value = newslug.replace(/ +/g, "-");
        console.log(slugValue.value);
    }
    if((key >= 65) && (key <= 90)) {
        key_str = String.fromCharCode(key);
        slugValue.value += key_str.toLowerCase();
        //console.log(slugValue.value);
    }
};
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Nice work! Here are some suggestions:

Input variable doesn't change, make it constant

I wouldn't imagine that you would want to reassign nameValue to something else, so it's best practice to keep the value a constant. The rule of thumb I use when programming in JavaScript is to always make a variable const, unless I know for sure that I need to mutate the value.

Using event listeners instead of on

nameValue.onkeyup works, but I would generally stay away from it because you can only execute one function/action for that element. Let's pretend I do:

$el.onclick = function() {
  console.log("number 1");
}

If I want to add click listener, I'd be doing:

$el.onclick = function() {
  console.log("number 2");
}

The second function overrides the first, and the first function no longer executes when clicked. While this may be occasionally useful, generally you'd want to stick to event listeners:

nameValue.addEventListener(function() { ... });

This way, however many events you register, they won't override each other.

Move the slug selector out of the function to prevent unnecessary execution

You're putting let slugValue = document.getElementById("slugValue"); inside the function, which will execute every time keyup is fired. This line is not dependent on any of the code within the function, so you're just wasting queries. Move it out of the function.

ALWAYS use declarators for variables

This isn't even allowed in strict mode. Declaring variables without declarators (you did it with key) will automatically define the variable in the browser's window object, and this can lead to unexpected/undesired behavior.

You can use key instead of keyCode

This makes it easier to manipulate keystrokes as it gives you the actual key name, like "Enter" instead of 13.

❗ You don't have to mess with keystrokes

Manually building a string based on individual keystrokes is a pain in the neck and more complex than you realize. What if the user pasted text or typed some text, went back a few characters, then typed some more text?

The much easier way to approach this is to listen for input change, then reprocess the whole value based on the new input value, something like...

nameValue.addEventListener("keyup", function() {
  const result = nameValue.value
    .toLowerCase()
    .replace(/ +/g, "-");
  slugValue.value = result;
});

Ensuring URL-safe slugs

Since you're generating slugs, you'll want to make sure that they're URL-friendly and won't cause the browser to do weird things when entered. You have two options.

The first one is to percent-encode the values to a URL safe format, which is quite simple:

slugValue.value = encodeURIComponent(result);

However, this may end up in an ugly symbol-congested URL, so it's customary to strip all the non-URL safe characters (or only keep the URL-safe characters). Since you're already using regex:

slugValue.value = [...result.match(/[\w\-]/g)].join("");

This will give us a nice-looking safe URL slug to work with.


And that's all I have. You're doing well! :)

\$\endgroup\$
2
  • \$\begingroup\$ Nice answer. I would also add "prefer to use === over ==, as you almost never want to intentionally use ==", and I would probably replace the sequential ifs with switch/case. It a little weird to have the code continue checking the same input if we found a match already. Even if/else would be preferable to the current structure. \$\endgroup\$ May 14 at 19:32
  • \$\begingroup\$ @MattMorgan sure, but I ignored that section (perhaps I shouldn't have) because the OP shouldn't even be using that section; it's all gone wrong. I would also venture to say that there are some, if not plenty of cases where using == could be preferable to === for speed's sake. \$\endgroup\$
    – code
    May 14 at 21:42

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.