Skip to content

Clean Code

Code is clean if it can be understood easily – by everyone on the team. 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

Every team should 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. These should be replaced with polymorphism." -- G5 Duplication

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

"If you have a constant such as a default or configuration value that is known and expected at a high level of abstraction, do not bury it in a low-level function."-- G35

Incorrect 
<!-- lots of other code here... -->
<mvt:assign name="l.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="l.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="l.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="l.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.