Style guide template engine

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,
'$&'));

$('.field-style .less')
.html($('.field-style .less')
.html()
.replace(
/(em)|(px)/g,
'$&'));
};

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

// Button m

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:


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

(Due to limitations on the representation of whitespace, I've seen myself forced to replace all tabs with a ` '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,
                '%%CODEBLOCK_8%%amp;'));

    $('.field-style .less')
        .html($('.field-style .less')
            .html()
            .replace(
                /(em)|(px)/g,
                '%%CODEBLOCK_8%%amp;'));
};

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,
                '%%CODEBLOCK_9%%amp;'
            )
            .replace(
                /(em)|(px)/g,
                '%%CODEBLOCK_9%%amp;'
            )
    );
}

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.

StackExchange Code Review Q#104210, answer score: 3