Style guide template engine

Question

I'm new to JS and am trying to write a perfectly clean source code. I'm using gulp to concatenate and minify my files, as well as removing comments. So the end result will always be the same in production environment: a single line without spaces. But for a dev environment, I'm trying to achieve clean code.

I'm building a small style guide template engine, based on Jekyll. But I need JS for several features, like dynamic styling of components. I might be overzealous but here is one of my modules with its source indentation and formatting:

$(function() {

    /* Dynamic Styling ------------------ */

    // Links
    $('.field-style a')
        .each(function() {
            var base =
                $(this)
                .next('pre')
                .children('code')
                .text();
            var replaced =
                base
                .replace(
                    /    |{|}|^a |\r?\n|\r/g,
                    '');
            $(this)
                .attr('style', replaced);
        });

    // Color box
    $('.field-style .colored')
        .each(function() {
            var base =
                $(this)
                .next('pre')
                .children('code')
                .text();
            var replaced =
                base
                .replace(
                    /\D+/g,
                    '');
            $(this)
                .css('background', '#' + replaced);
        });

    // LESS code blocks
    if ($('.field-style .less')
        .length) {
        $('.field-style .less')
            .html($('.field-style .less')
                .html()
                .replace(
                    /(none)|(decimal)|(inside)|(hidden)/g,
                    '<span class="value">$&</span>'));

        $('.field-style .less')
            .html($('.field-style .less')
                .html()
                .replace(
                    /(em)|(px)/g,
                    '<span class="unit">$&</span>'));
    };

    /* Code blocks trigger ------------------ */

    // Button markup
    var button =
        '<button type="button" href="javascript:;"' +
        'class="btn btn-primary show-code">' +
        'Voir CSS</button>';

    // Button generator
    $('pre')
        .before(button);

    // Animation
    $('.show-code')
        .on('click', function() {
            $(this)
                .next('pre')
                .slideToggle();
            return false;
        });

});

As you can see, I indented every method as well as every var. I also break lines in replace method for each argument

What would you say about this indentation level?

Answer 1

While looking at the code, the first thing that poped on me was the indentation. It is too strict, too inflexible, context-less, width-less... It isn't a good indentation.

Right in the middle of your code, you have this:

// LESS code blocks
if ($('.field-style .less')
    .length) {

This is next to unreadable. I even though it had broken syntax. Here's how it should be:

// LESS code blocks
if ($('.field-style .less').length) {

The { on the same line is target of discussion. In my opinion, it should be in a line on it's own but most people prefer this way.

Other things like this:

$('.field-style a')
    .each(function() {

Which can and should be all in the same line. You should only split the methods in new lines if and only if they look too wide.

---

Now, lets split it up and analise:

// Links
$('.field-style a')
    .each(function() {
        var base =
            $(this)
            .next('pre')
            .children('code')
            .text();
        var replaced =
            base
            .replace(
                /    |{|}|^a |\r?\n|\r/g,
                '');
        $(this)
            .attr('style', replaced);
    });

You re-use the $(this) in there, and store the text on a variable. Your priorities are swapped! The slowest thing in that code is repeated, but you have to store a string in a new variable.

This is how I would write it:

$('.field-style a').each(function() {
    var $this = $(this);

    var replaced = $this
        .next('pre')
        .children('code')
        .text()
        .replace(/    |{|}|^a |\r?\n|\r/g, '');

    $this.attr('style', replaced);
});

You could even write it as:

$('.field-style a').each(function() {
    var $this = $(this);

    $this.attr(
        'style',
        $this
            .next('pre')
            .children('code')
            .text()
            .replace(/    |{|}|^a |\r?\n|\r/g, '')
    );
});

This part can also be used for the $('.field-style .colored') block.

But a major concerning area is that regular expression. It is made to match newlines, the first a and a space and {}. Here's my proposal:

/a |\s*(?:\r|\n|\r\n)\s*|[\{\}]/g

This goes a step further and also deletes any tab or any other whitespace after and before a newline.

An input like:

a {
    color: red;
    size: auto;
    width: 120px; 
    white-space: break;
}

Will result in:

color: red;size: auto;width: 120px;white-space: break;

While yours results in:

<TAB>color: red;size: auto;<TAB>width: 120px; white-space: break;

_{(Due to limitations on the representation of whitespace, I've seen myself forced to replace all tabs with a <TAB> 'indicator')}

---

Lets analise the .less block:

// LESS code blocks
if ($('.field-style .less')
    .length) {
    $('.field-style .less')
        .html($('.field-style .less')
            .html()
            .replace(
                /(none)|(decimal)|(inside)|(hidden)/g,
                '<span class="value">$&</span>'));

    $('.field-style .less')
        .html($('.field-style .less')
            .html()
            .replace(
                /(em)|(px)/g,
                '<span class="unit">$&</span>'));
};

First, you have that dangling ;.

You are currently wasting too much CPU! You repeated $('.field-style .less') 5 times! You have no idea how slow that is. Store that in a new variable.

You could remove repetitions, like this:

var $less = $('.field-style .less');
if( $less.length ) {
    $less.html(
        $less
            .html()
            .replace(
                /(none)|(decimal)|(inside)|(hidden)/g,
                '<span class="value">$&</span>'
            )
            .replace(
                /(em)|(px)/g,
                '<span class="unit">$&</span>'
            )
    );
}

But I'm still not happy. The regex don't need the () around every word. Or at all. Basically, you could write both as this:

/none|decimal|inside|hidden/g
/em|px/g

In the replacements, instead of $&, you use $0. This will replace everything that was matched.

---

You currently have the following code:

// Animation
$('.show-code')
    .on('click', function() {
        $(this)
            .next('pre')
            .slideToggle();
        return false;
    });

There's not much to say on this one. Using the .on() handler, you can take advantage of it by picking a common parent node. Then you set the 2nd argument to have a selector. Like this:

$('#fake-parent').on('click', '.show-code', function(){
    [...]
});

Also, notice that when you use return false; on an event handler, jQuery runs event.preventDefault() and event.preventPropagation(). Depending on what you are trying to do, you should be careful when returning anything in an event handler.

Answer 2

I recommend you to stick with one of well known and widely used style guides, such as Airbnb or Google guides. As an example:

18.6 Use indentation when making long method chains. Use a leading dot, which emphasizes that the line is a method call, not a new statement.

You can see the rest here: Airbnb JavaScript Style Guide