3
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I edited the regex you suggested with : (/a \{|\s*(?:\r|\n|\r\n)\s*|\}/g, '') , it seemed more suited to my html. With the ^ operator, the first a wasn't selected \$\endgroup\$ – Antonin Cezard Sep 9 '15 at 13:21
  • \$\begingroup\$ @topleft Sorry the delay in answering to your comment. I've quickly edited the regular expression. If there's something else that isn't accurate to your, feel free to comment and I'll answer as soon as possible. \$\endgroup\$ – Ismael Miguel Sep 9 '15 at 14:52
0
\$\begingroup\$

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

\$\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.