Skip to content

Clean Code#

Code is clean if it can be understood easily. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.

This notion of of “Clean Code” and the topics below are excerpts & concepts from Clean Code: A Handbook of Agile Software Craftsmanship by “Uncle Bob“

General rules#

Follow Standard Conventions#

Follow a coding standard based on common industry norms.

We’ve defined our code standards here

(KISS) Keep It Simple Stupid#

Simpler is always better. Reduce complexity as much as possible.

“Everything should be made as simple as possible, but not simpler.” – Albert Einstein

(DRY) Don’t Repeat Yourself#

“Every time you see duplication in the code, it represents a missed opportunity for abstraction.

The most obvious form of duplication is when you have clumps of identical code that look like some programmers went wild with the mouse, pasting the same code over and over again. These should be replaced with simple methods.

A more subtle form is the switch/case or if/else chain that appears again and again in various modules, always testing for the same set of conditions.

The Boy Scout Rule#

Leave the campground cleaner than you found it.

“It’s not enough to write the code well. The code has to be kept clean over time. We’ve all seen code rot and degrade as time passes. So we must take an active role in preventing this degradation.

If we all checked-in our code a little cleaner than when we checked it out, the code simply could not rot.”

Always Find Root Cause#

Always look for the root cause of a problem instead of applying a band-aid or quick-fix.

Quick fixes have a tendency of remaining in production indefinitely so do it right the first time.

Design rules#

Keep Configurable Data At High Levels#

Incorrect

<!-- lots of other code here... -->
<mvt:assign name="s.null" value="file_overwrite( '/path/to/export/file.txt', 'data', l.file_data )">

Correct

<mvt:assign name="l.EXPORT_FILE_PATH" value="'/path/to/export/file.txt'">
<!-- lots of other code here... -->
<mvt:assign name="s.null" value="file_overwrite( l.EXPORT_FILE_PATH, 'data', l.file_data )">

Prevent Over-Configurability#

Decrease the number of decisions that a developer using the framework, class, function, etc. is required to make without losing flexibility.

Further Reading:

Understandability tips#

Be consistent#

If you do something a certain way, do all similar things in the same way.

For example, use the same vocabulary for the same type of variable.

Incorrect

<mvt:assign name="l.user_first_name" value="'John'" />
<mvt:assign name="l.shopper_last_name" value="'Doe'" />
<mvt:assign name="l.customer_full_name" value="l.user_first_name $ ' ' $ l.shopper_last_name" />

Correct

<mvt:assign name="l.customer:first_name" value="'John'" />
<mvt:assign name="l.customer:last_name" value="'Doe'" />
<mvt:assign name="l.customer:full_name" value="l.customer:first_name $ ' ' $ l.customer:last_name" />

Use Explanatory Variables#

One of the more powerful ways to make a program readable is to break the calculations up into intermediate values that are held in variables with meaningful names.

Example with Concatenation:

Incorrect

<mvt:assign name="l.formatted_date" value="time_t_year( s.time_t, 'local' ) $ '-' $ padl(time_t_month( s.time_t, 'local' ), 2, 0) $ '-' $ padl(time_t_dayofmonth( s.time_t, 'local' ), 2, 0) $ ' ' $ padl(time_t_hour( s.time_t, 'local' ), 2, 0) $ ':' $ padl(time_t_min( s.time_t, 'local' ), 2, 0) $ ':' $ padl(time_t_sec( s.time_t, 'local' ), 2, 0)" />

Correct

<mvt:assign name="l.timezone" value="'local'" />
<mvt:assign name="l.year" value="time_t_year( s.time_t, l.timezone )" />
<mvt:assign name="l.month" value="padl(time_t_month( s.time_t, l.timezone ), 2, 0)" />
<mvt:assign name="l.day" value="padl(time_t_dayofmonth( s.time_t, l.timezone ), 2, 0)" />
<mvt:assign name="l.24hour" value="padl(time_t_hour( s.time_t, l.timezone ), 2, 0)" />
<mvt:assign name="l.min" value="padl(time_t_min( s.time_t, l.timezone ), 2, 0)" />
<mvt:assign name="l.sec" value="padl(time_t_sec( s.time_t, l.timezone ), 2, 0)" />

<mvt:assign name="l.formatted_date" value="l.year $ '-' $ l.month $ '-' $ l.day $ ' ' $ l.24hour $ ':' $ l.min $ ':' $ l.sec" />

Example with Complex Conditionals:

Incorrect

<mvt:if expr="l.settings:page:code EQ 'LOGN' OR l.settings:page:code EQ 'ORDL' OR l.settings:page:code EQ 'GFTL'">
    <!-- Do something for these pages -->
</mvt:if>

Correct

<mvt:assign name="l.is_login_page" value="l.settings:page:code EQ 'LOGN' OR l.settings:page:code EQ 'ORDL' OR l.settings:page:code EQ 'GFTL'">
<mvt:if expr="l.is_login_page">
    <!-- Do something for the login pages -->
</mvt:if>

Avoid Hidden Dependencies#

Don’t write code that assumes other/global dependencies are available. Either load the dependency yourself or make the dependency a parameter that is passed to it.

Further reading: How to eliminate (or deal with) hidden dependencies

Avoid Negative Conditionals#

Negatives are just a bit harder to understand than positives. So, when possible, conditionals should be expressed as positives.

Incorrect

<mvt:assign name="l.customer_is_logged_out" value="g.Basket:cust_id EQ 0">
<mvt:if expr="NOT l.customer_is_logged_out">
    <!-- Do something for logged in customers -->
</mvt:if>

Correct

<mvt:assign name="l.customer_is_logged_in" value="g.Basket:cust_id GT 0">
<mvt:if expr="l.customer_is_logged_in">
    <!-- Do something for logged in customers -->
</mvt:if>

Names Rules#

Choose Descriptive And Unambiguous Names#

Names are 90% of what makes code readable. Take the time to choose them wisely and keep them relevant.

Incorrect

const calc = (a, b) => {
    const c = a - b;
    const d = c / a;
    const e = d * 100;
    const f = Math.round(e) + "%";
    return f;
};

Correct

const determineSavings = (basePrice, price) => {
    const difference = basePrice - price;
    const ratio = difference / basePrice;
    const percent = ratio * 100;
    const formatted = Math.round(percent) + "%";
    return formatted;
};

Make Meaningful Distinctions#

Entities named differently, should actually be different and correspond to different things.

See the examples from Be consistent.

Additionally, if the code-base has key-terms, data-structures, or conventions, then use them intentionally or avoid them when there is a difference. For example:

Incorrect

<mvt:item name="customfields" param="Read_Product_ID( l.settings:product:id, '', l.settings:product:fields )" />
<mvt:assign name="l.settings:product:price" value="l.settings:product:fields:lowest_price $ ' to ' $ l.settings:product:fields:highest_price">

Correct

<mvt:item name="customfields" param="Read_Product_ID( l.settings:product:id, '', l.settings:product:customfield_values:customfields )" />
<mvt:assign name="l.settings:product:price_range" value="l.settings:product:customfield_values:customfields:lowest_price $ ' to ' $ l.settings:product:customfield_values:customfields:highest_price">

Use Pronounceable Names#

Incorrect

<mvt:do file="g.Module_Library_DB" name="l.success" value="Product_Load_Code( 'CODE', l.settings:prod )" />

Correct

<mvt:do file="g.Module_Library_DB" name="l.success" value="Product_Load_Code( 'CODE', l.settings:product )" />

Use Searchable Names#

Avoid abbreviated and single letter variables as they are difficult to search for and comprehend.

See Choose descriptive and unambiguous names for another example.

Incorrect

const elms = document.getElementsByClassName(".js-tooltips");
for (const elm of elms) {
    elm.addEventListener("click", () => openTooltip(elm));
}

Correct

const tooltips = document.getElementsByClassName(".js-tooltips");
for (const tooltip of tooltips) {
    tooltip.addEventListener("click", () => openTooltip(tooltip));
}

Replace Magic Numbers With Named Constants#

Incorrect

<mvt:assign name="s.null" value="miva_sleep( 60000 )">

Reasoning: We do not know what 60000 stands for

Correct

<mvt:assign name="l.MILLISECONDS_IN_A_MINUTE" value="60000">
<mvt:assign name="s.null" value="miva_sleep( l.MILLISECONDS_IN_A_MINUTE )">

Note: The variable was declared with the Constant naming convention

Avoid Encodings#

Don’t append/prefix variables with data-type information.

Incorrect

const determineSavings = (basePriceNumber, priceNumber) => {
    const differenceNumber = basePriceNumber - priceNumber;
    const ratioNumber = differenceNumber / basePriceNumber;
    const percentFloat = ratioNumber * 100;
    const formattedString = Math.round(percentFloat) + "%";
    return formattedString;
};

Correct

const determineSavings = (basePrice, price) => {
    const difference = basePrice - price;
    const ratio = difference / basePrice;
    const percent = ratio * 100;
    const formatted = Math.round(percent) + "%";
    return formatted;
};

Functions Rules#

Small#

“The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that”

Our ESLint settings has an “enforce a maximum function length” rule which defaults to 50 lines. Keep other languages (PHP, MivaScript, etc.){:target=_blank} to about 50 lines as well.

Do One Thing#

“Functions should do one thing. They should do it well. They should do it only.”

“If a function does only the steps that are one-level below the stated name of the function, then the function is doing one thing.”

Incorrect

const addToCart() {
// Code to set processing

// Code to AJAX Add To Cart

// Code to handle AJAX call response

// Code to update minibasket

// 200 lines of code...
}

Correct

const setProcessing = (form) => {
    // Code to set processing
}

const ajaxAddToCart = (form) => {
    // Code to build ajax call
}

const handleAjaxCallResponse = (response) => {
    // Code to handle AJAX call response

    if( response.success ){
        updateMinibasket(response);
    }
}

const updateMinibasket = (response) => {
    // Code to update minibasket
}

const addToCart() {
    setProcessing(form);
    ajaxAddToCart(form).then((response) => {
        handleAjaxCallResponse(response);
    });
}

Use Descriptive Names#

See the Choose descriptive and unambiguous names

Prefer Fewer Arguments#

When you think you need more than three arguments, that’s typically a good time to move to a single “options” object that is passed in.

As you have more arguments, it becomes harder to know the proper order to set them in. Objects allow the arguments to be labeled and put in any order.

Incorrect

const openModal = (trigger, content, style, beforeOpen, afterOpen, beforeClose, afterClose) => {
    // Do something...
};

openModal(".js-open-modal", "#js-modal-content", "compact", null, null, confirmClose, handleAfterClose);

Correct

const openModal = (options) => {
    // Do something...
};

openModal({
    afterClose: handleAfterClose,
    beforeClose: confirmClose,
    content: "#js-modal-content",
    style: "compact",
    trigger: ".js-open-modal",
});

Have No Side Effects#

“Side effects are lies. Your function promises to do one thing, but it also does other hidden things.”

In other words, use Pure Functions A pure function is a function which:

  1. Given the same input, always returns the same output
  2. Produces no side effects

Incorrect

let total = 0;

const increaseTotal = (amount) => {
    total += amount; // Side effect: mutates instance outside scope of function.
    return total;
};

increaseTotal(10); // => 10; Given 10 it returns 10;
increaseTotal(10); // => 20; Given 10 again, it returns a different value: 20

Correct

let total = 0;

const add = (a, b) => {
    return a + b;
};

total = add(total, 10); // => 10; basically the same as sum(0, 10)
total = add(total, 10); // => 20; basically the same as sum(10, 10) -- Different input yields different output

Don’t Use Flag Arguments#

Split method into several independent methods that can be called without the flag.

Incorrect

function createFile(name, isTemp) {
    if (isTemp) {
        fs.create(`./temp/${name}`);
    } else {
        fs.create(name);
    }
}

Correct

function createFile(name) {
    fs.create(name);
}

function createTempFile(name) {
    createFile(`./temp/${name}`);
}

Comments Rules#

“Don’t comment bad code – rewrite it!” – Brian W. Kernighan and P.J. Plaugher

“The proper use of comments is to compensate for our failure to express ourself in code. So when you find yourself in a position where you need to write a comment, think it through and see whether there isn’t some way to turn the tables and express yourself in code.”

Explain Yourself In Code First#

Incorrect

// Check to see if the customer is eligible for free shipping
if (customer.isLoggedIn && basket.total > 100) {
    // Do something
}

Correct

if (isEligibleForFreeShipping()) {
    // Do something
}

Don’t Be Redundant#

See Comments > Use comments to explain areas the code can not.

Avoid Closing Tag/Brace Comments#

Closing tag/brace comments can add unnecessary clutter to the code base and over time they can often become out-of-sync.

Instead, use proper indentation, code-folding in your IDE, or refactor long functions & code blocks to be less lines.

Incorrect

<div class="x-product-card">
    <a href="&mvte:product:link;">
        <img class="lazyload" src="&mvte:image_placeholders:default;" data-src="&mvte:product:imagetypes:main;" alt="" />
        <div class="x-product-card__name">&mvte:product:name;</div>
    </a>
</div>
<!-- END .x-product-card -->

Correct

<div class="x-product-card">
    <a href="&mvte:product:link;">
        <img class="lazyload" src="&mvte:image_placeholders:default;" data-src="&mvte:product:imagetypes:main;" alt="" />
        <div class="x-product-card__name">&mvte:product:name;</div>
    </a>
</div>

Don’t Comment Out Unused Code. Just Remove It.#

Do not use comments to remove functionality (fine for testing/troubleshooting). Code that is not intended to ever be run should be removed.

Move the functionality to documentation or remove it and rely on the git history.

Comments Explain The Why; Not The What#

Use comments to clarify your intentions with code rather then explaining what the code does.

See Comments > Use comments to explain areas the code can not

Use As Warning Of Consequences#

Use comments to warn developers of potential issues or future problems.

Incorrect

<mvt:comment>
|
|   Load Subcategory data for them to be used in: `subcategory_tree` & 'subcategories` RT CS
|
</mvt:comment>
<mvt:if expr="g.Screen EQ 'CTGY' OR l.settings:page:code EQ 'CTLG'">

    <mvt:if expr="l.settings:page:code EQ 'CTLG'">
    <mvt:assign name="l.settings:category" value="''" />
    </mvt:if>

    <mvt:if expr="ISNULL l.settings:category:subcategories">
        <mvt:do file="g.Module_Library_DB" name="l.settings:category:subcategory_count" value="Runtime_CategoryList_Load_Parent(l.settings:category:id, l.settings:category:subcategories)" />
    </mvt:if>

    <!-- Do more things -->

</mvt:if>

Correct

<mvt:comment>
|
|   Load Subcategory data for them to be used in: `subcategory_tree` & 'subcategories` RT CS
|
</mvt:comment>
<mvt:if expr="g.Screen EQ 'CTGY' OR l.settings:page:code EQ 'CTLG'">

    <mvt:comment>
    |
    |   This is used to clear the `l.settings:category` variable because:
    |   Miva's Smart Breadcrumbs will populate that variable when the s.http_referer matches a category page
    |   even if the current page is not a CTGY page for an existing category (i.e. CTLG)
    |   Populating that variable breaks the expected `Runtime_CategoryList_Load_Parent`
    |   and loads the s.http_referer's subcategories instead of the top-level categories; like we want
    |
    </mvt:comment>
    <mvt:if expr="l.settings:page:code EQ 'CTLG'">
    <mvt:assign name="l.settings:category" value="''" />
    </mvt:if>

    <mvt:if expr="ISNULL l.settings:category:subcategories">
        <mvt:do file="g.Module_Library_DB" name="l.settings:category:subcategory_count" value="Runtime_CategoryList_Load_Parent(l.settings:category:id, l.settings:category:subcategories)" />
    </mvt:if>

    <!-- Do more things -->

</mvt:if>

Source Code Structure#

Separate Concepts Vertically#

Group related topics together and separate lines that are new or unrelated topics. Related code should appear vertically dense.

Incorrect

<head>
    <mvt:item name="head" param="head_tag" />
    <mvt:capture variable="l.product_display_imagemachine">
        <mvt:item name="product_display_imagemachine" param="head_deferred" />
        <mvt:item name="product_display_imagemachine" param="body_deferred:product:id"/>
    </mvt:capture>
    <mvt:capture variable="l.attributemachine">
        <mvt:item name="attributemachine" param="head_deferred" />
        <mvt:item name="attributemachine" param="body_deferred" />
    </mvt:capture>
    <mvt:assign name="l.settings:product:social:description"    value="'Check out the deal on ' $ l.settings:product:name $ ' at ' $ g.store:name" />
    <mvt:assign name="l.settings:product:social:image"          value="g.secure_baseurl $ l.settings:product:customfield_values:productimagecustomfields:main" />
    <mvt:assign name="l.settings:product:social:url"            value="l.settings:product:link" />
    <meta property="og:description" content="&mvte:product:social:description;">
    <meta property="og:image"       content="&mvte:product:social:image;">
    <meta property="og:image:alt"   content="&mvte:product:name;">
    <meta property="og:site_name"   content="&mvte:global:store:name;">
    <meta property="og:title"       content="&mvte:product:name;">
    <meta property="og:type"        content="product">
    <meta property="og:url"         content="&mvte:product:link;">
    <mvt:if expr="l.settings:twitter:username">
        <meta name="twitter:creator"    content="@&mvte:twitter:username;">
        <meta name="twitter:site"       content="@&mvte:twitter:username;">
    </mvt:if>
</head>

Correct

<head>
<mvt:item name="head" param="head_tag" />

    <mvt:capture variable="l.product_display_imagemachine">
        <mvt:item name="product_display_imagemachine" param="head_deferred" />
        <mvt:item name="product_display_imagemachine" param="body_deferred:product:id"/>
    </mvt:capture>

    <mvt:capture variable="l.attributemachine">
        <mvt:item name="attributemachine" param="head_deferred" />
        <mvt:item name="attributemachine" param="body_deferred" />
    </mvt:capture>

    <mvt:assign name="l.settings:product:social:description"    value="'Check out the deal on ' $ l.settings:product:name $ ' at ' $ g.store:name" />
    <mvt:assign name="l.settings:product:social:image"          value="g.secure_baseurl $ l.settings:product:customfield_values:productimagecustomfields:main" />
    <mvt:assign name="l.settings:product:social:url"            value="l.settings:product:link" />

    <meta property="og:description" content="&mvte:product:social:description;">
    <meta property="og:image"       content="&mvte:product:social:image;">
    <meta property="og:image:alt"   content="&mvte:product:name;">
    <meta property="og:site_name"   content="&mvte:global:store:name;">
    <meta property="og:title"       content="&mvte:product:name;">
    <meta property="og:type"        content="product">
    <meta property="og:url"         content="&mvte:product:link;">

    <mvt:if expr="l.settings:twitter:username">
        <meta name="twitter:creator"    content="@&mvte:twitter:username;">
        <meta name="twitter:site"       content="@&mvte:twitter:username;">
    </mvt:if>
</head>

Declare Variables Close To Their Usage#

Incorrect

onReady () {

    const cvvElem = document.querySelector('[data-id="cvv"]');

    new SummaryEvents();
    this.initPromoCodeEvents();
    this.creditCardDetection();

    /**
     * Added functionality to help style the default Miva output payment
     * fields.
     */
    if (cvvElem) {
        const cvvInput = cvvElem.querySelector('input');

        if (cvvInput) {
            cvvInput.setAttribute('type', 'tel');
        }
    }

}

Correct

onReady () {
new SummaryEvents();
this.initPromoCodeEvents();
this.creditCardDetection();

    /**
     * Added functionality to help style the default Miva output payment
     * fields.
     */
    const cvvElem = document.querySelector('[data-id="cvv"]');

    if (cvvElem) {
        const cvvInput = cvvElem.querySelector('input');

        if (cvvInput) {
            cvvInput.setAttribute('type', 'tel');
        }
    }

}

Dependent Functions Should Be Close#

“If one function calls another, they should be vertically close, and the caller should be above the callee, if at all possible.”

Incorrect

export default class AjaxAddToCart {
    constructor() {
        this.bindEvents();
    }

    onSubmit(e) {
        // Do something
    }

    // Lots of other code...

    bindEvents() {
        this.formElement.addEventListener("submit", this.onSubmit.bind(this), false);
    }
}

Correct

export default class AjaxAddToCart {
    constructor() {
        this.bindEvents();
    }

    bindEvents() {
        this.formElement.addEventListener("submit", this.onSubmit.bind(this), false);
    }

    onSubmit(e) {
        // Do something
    }

    // Lots of other code...
}

Similar Functions Should Be Close#

If a group of functions all perform a similar or related task, it is best to group them together.

Incorrect

export default class Message {
    setErrorMessage() {}

    open() {}

    setSuccessMessage() {}

    close() {}

    setWarningMessage() {}
}

Correct

export default class Message {
    open() {}

    close() {}

    setSuccessMessage() {}

    setErrorMessage() {}

    setWarningMessage() {}
}

Place Functions In The Downward Direction#

The Newspaper Metaphor: We would like a source file to be like a newspaper article. The name should be simple but explanatory. The name, by itself, should be sufficient to tell us whether we are in the right module or not. The topmost parts of the source file should provide the high-level concepts and algorithms. Detail should increase as we move downward, until at the end we find the lowest level functions and details in the source file. –Clean Code - Quotes: 5. Formatting

Keep Lines Short#

Strive to keep lines as horizontally short as possible/necessary. We don’t have a hard limit set, but 80-120 characters is a solid target.

Don’t Break Indentation#

“It is sometimes tempting to break the indentation rule for short if statements, short while loops, or short functions” Just avoid doing it when the temptation occurs.

Incorrect

function handleError(error) {
    console.error(error);
}

if (error) handleError(error);

Correct

function handleError(error) {
    console.error(error);
}

if (error) {
    handleError(error);
}

Classes#

See “Clean Code JavaScript”#

Clean Code JavaScript’s - Objects & Data Structures has some great examples for:

Follow “Law of Demeter”#

As an analogy, when one wants a dog to walk, one does not command the dog’s legs to walk directly; instead one commands the dog which then commands its own legs.

Class a can call a method of class b, but class a should not “reach through” class b to access yet another class c, to request its services. Doing so would mean that class a implicitly requires greater knowledge of class b’s internal structure.

  • Each unit should have only limited knowledge about other units: only units “closely” related to the current unit.
  • Each unit should only talk to its friends; don’t talk to strangers.

– From Wikipedia

Hide internal structure#

Use Private Class Fields and methods to share internal structures.

Incorrect

class User {
    name;
    constructor(name) {
        this.name = name;
    }
}
const user = new User("Jon Snow");
user.name; // => 'Jon Snow'

Correct

class User {
    #name;
    constructor(name) {
        this.#name = name;
    }
    getName() {
        return this.#name;
    }
}
const user = new User("Jon Snow");
user.getName(); // => 'Jon Snow'
user.#name; // SyntaxError is thrown

Should Be Small#

Similar in concept to Functions > Small

Do One Thing#

Similar in concept to Functions > Small

Tests#

The same things that makes production code readable, makes tests readable: clarity, simplicity, and density of expression. See the rest of this clean-code page for more details.

FIRST Rules#

Tests should be Fast, Independent, Repeatable, Self-Validating, and Timely.

Fast#

Tests should be Fast.

They should run quickly. When tests run slow, you won’t want to run them frequently. If you don’t run them frequently, you won’t find problems early enough to fix them easily. You won’t feel as free to clean up the code. Eventually the code will begin to rot.

Independent#

Tests should be Independent of other tests.

Tests should not depend on each other. One test should not set up the conditions for the next test. You should be able to run each test independently and run the tests in any order you like.

Repeatable#

Tests should be Repeatable in any environment.

You should be able to run the tests in the production environment, in the QA environment, and on your laptop while riding home on the train without a network. If your tests aren’t repeatable in any environment, then you’ll always have an excuse for why they fail.

Self-Validating#

Tests should have a boolean output.

Either they pass or they fail. You should not have to read through the log file to tell whether the tests pass. You should not have to manually compare two different text files to see whether the tests pass.

Timely#

Tests should be written in Timely fashion.

Tests should be written just before production code that makes them pass. If you write the tests after the production code, then you may find the production code to be hard to test. You may decide that some production code is too hard to test.

Code Smells#

Rigidity#

The software is difficult to change. A small change causes a cascade of subsequent changes.

Fragility#

The software breaks in many places due to a single change.

Immobility#

You cannot reuse parts of the code in other projects because of involved risks and high effort.

Needless Complexity#

When a design is more complicated than it needs to be.

Needless Repetition#

Similar code or code in slightly different forms appears in different places.

Opacity#

Difficult to understand the code.