JS/JSX Styleguide

[JobLeonard]

So I'm reformatting/refactoring the whole JS side of the project using the linting/style rules I settled on. A lot of stuff is automatically fixable by the linter, but some style guide options require active maintenance by the programmer.

To be clear: this is just a proposal at the moment! I'm following the rules myself, but they're open for discussion.

... but as I'm writing this I'm starting to realise that I'm putting way too much thought into this.

One of the main ideas I have is based on what I've learned about human brain capacity, which can be summed up as follows:

Human working memory is generally limited to about four items. The brain uses chunking (in short: grouping items and keeping that group in memory as one item) to overcome this limitation.

Following that insight, we can expect that structuring code in such a way that it represents sensible "chunks" will make it more readable and easier to work with.

Here are a few fragments of code showing what I think we maintains a good trade-off in "information density", please let me know if you have more thoughts on that:

const optionNames = ["Text", "Bars", "Heatmap", "Categorical"];
const showOptionsForRows = optionNames.map((name) => {
    return (
        <li key={name}>
            <a onClick={
                () => { dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name }); }
            }>
                {name}
            </a>
        </li>
    );
});
const showOptionsForCols = optionNames.map((name) => {
    return (
        <li key={name}>
            <a onClick={
                () => { dispatch({ type: 'SET_HEATMAP_PROPS', colMode: name }); }
            }>
                {name}
            </a>
        </li>
    );
});

<LoomTextEntry
    trimUnderscores={true}
    className='col-sm-10'
    defaultValue=''
    componentUpdateHandler={this.handleFormUpdate}
    name='transcriptome'
    id='input_transcriptome'
    />

Always wrap code blocks in {}

• Always use if (...) {}, even for one-liners
• Always use () => {} with Fat Arrow syntax, even for small functions

First, consistency helps readability. More importantly, also makes expanding existing code less bug-prone, since the enclosing { and } will already be there.

As a general rule of thumb, have at most two to three "things" happen per line

To be clear, "things happen" can also refer to plain information in this context, like a big array of data.

Aside from the obvious refactoring of long code fragments into functions to structure code, there are a few other ways to achieve this:

Rule-of-thumb: JS and JSX on separate lines unless really light on code density

<a onClick={
    () => { dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name }); }
}>
    {name}
</a>

This helps readability, since we don't have to switch "languages" per line.

As another benefit, this will save horizontal space (at the cost of vertical space, but horizontal space is more precious IMO). Finally the added indentation structure makes the flow of the code more visible.

Following this principle, return should always be in the following form:

return(
    <JSX code>
);

Note that this also mirrors the rule that code blocks should always be wrapped in {}

JSX: Split prop entries over separate lines

The <LoomTextEntry /> illustrates this. Fairly straightforward.

JS: Prefer splitting code out into separate statements over having really dense one-liners

We read code per line, so the amount of information per line matters. With two or three items you still have some "brain memory" left to keep track of whatever you are actually working on when scanning the code.

JS: Use clearly labelled intermediate variables to chunk information

It's actually a bit more subtle than just "chunking" information in a variable; this might be easier to explain with an example. The following code was what I originally found the source:

//WRONG: TOO MUCH HAPPENING
var showOptionsForRows = ["Text", "Bars", "Heatmap", "Categorical"].map((name) => { 
    /*...*/ 
});

This line clearly goes beyond the 4-items limit of our brain; not just in simple item-count, but also in types of information. There's the array, it's contents, what is done with it (anonymous function) and how (map), and finally the variable it is assigned to. While all connected, these end up distracting from each other when shown all at the same time, because we have to "switch out" items in our working memory.

The most obvious way to divide this into multiple lines is assigning the array to a variable, for two reasons:

• If I want to know what's in the array, I have to ignore the rest of the line.
• Vice versa, if I want to know what is being done with the array, I have to ignore the contents of the array.

Giving the array its own variable makes it easier to focus on either task:

const optionNames = ["Text", "Bars", "Heatmap", "Categorical"];
const showOptionsForRows = optionNames.map((name) => { 
    /*...*/ 
});

• optionNames labels ("chunks") the array so we can easily refer to it without worrying about the contents,
• vice versa we can manipulate the array's contents without looking at what is done with it.

It should be noted that we also reuse optionNames later in the code, so this makes refactoring easier as well. (Also, since I know neither optionNames nor showOptionsForRows changes, I made it a const. The linter no longer allows var - everything is either const or let, to enforce more sensible scoping rules)

In short: splitting out code into lines, combined with indentation (see below), will "chunk" the code through visual structure, freeing up the working memory to actually understand the code.

JS/JSX: When in doubt, choose that which autoformats to a consistent indentation tree

This complements the previous rule: indentation helps with connecting opening and closing sections of code, visually chunking it for us, so maintaining a logical indentation is important.

This is why the <li> and` tags are on separate lines: it makes it clear that the latter is a child of the former.

Writing in a style so that the autoformatter creates a consistent tree structure will also help finding bugs, since any deviations from the norm are likely to be the result of a forgotten code fragment, and thus a bug that needs to be removed.

For this reason I settled on K&R indentation-style for brace placement (Wiki-article on indent style, possibly the ultimate piece of bike-shedding). The preference for K&R here is for the following reasons:

• JS' crazy semicolon insertion rules mean you should never put an opening { on a different line, unless you love hard-to-find bugs
• Having the closing } match indentation depth with the line that contains the opening { creates vertical consistency

For example, let's look at the following <a>-tag with an anonymous function:

// CORRECT IN THIS CONTEXT
// =======================

<a onClick={ 
    () => { dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name }); }
}>

It's a bit awkward, but probably the best we can do. <a onClick={has a<and{, so their closing counterparts should be on the same line as}>to match the indentation. The anonymous function() => { /.../ }` is on a single line and somewhat pushing the four-item rule.

// POTENTIALLY CORRECT ALTERNATIVES
// ================================

// Makes sense if there are multiple statements, instead of a single 
// call to dispatch() - although refactoring into a wrapper function 
// might make more sense here (JS inlines quite decently)

<a onClick={
    () => {
        dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name });
    }
}>

// Makes sense if call to dispatch() would take multiple parameters 
// or a large JSON object, but again: refactoring into wrapper
// function and calling that probably makes more sense.

<a onClick={
    () => {
        dispatch(
            { type: 'SET_HEATMAP_PROPS', rowAttr: name }
        );
    }
}>

<a onClick={
    () => {
        dispatch({
            type: 'SET_HEATMAP_PROPS',
            rowMode: name,
        });
    }
}>

The first example works, especially for longer anonymous functions, but pushes the split a bit too far here: the only advantage is separating the two closing brackets - unlike the example above I think we can handle three opening/closing symb}); }

The second/third examples are for complicated function calls, but at that point Compare to other alternatives look like this (when using the autoformatter):

// WRONG: TOO MUCH STUFF HAPPENING IN A SINGLE LINE
// ================================================

<a onClick={ () => { dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name }); } }>

<a onClick={ () => {
    dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name });
} }>

The issue in the first example is too many things happening in one line. Also, it is easy to miss a } in those closing }); } }> or to pick the wrong one when rewriting the code. Likely to cause more bugs when rewritten!

The second example is better, but still has three closing } }> on a single line. Even with IDE highlighting matching opening/closing symbols I find that tiresome on the eyes.

// WRONG: BREAKS INDENTATION CONSISTENCY
// ======================================
<a onClick=
    {
        () => {
            dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name });
        }
    }
    >

//WRONG: BREAKS INDENTATION CONSISTENCY
<a onClick={ () => {
    dispatch({ type: 'SET_HEATMAP_PROPS', rowMode: name });
}
}>

The two last lines feel structurally confusing to me, as if the first } was a forgotten piece of code after a rewrite that had to be removed.

Other style rules

• No var, only let or const
• Use autoformatter to enforce standardised spacing rules (also, we use tabs for intendation)
• Avoid unused variables (merely a warning by the linter)
• Don't be afraid to delete commented out code
• Arrays and JSON object split over multiple lines have a dangling comma

let formData = [
    'col_attrs',
    'row_attrs',
    'n_features',
    'cluster_method',
    'regression_label',
];

let newState = {
    droppedFile: file,
    fileName: file.name,
    fileIsCSV: file.type === "text/csv",
    fileSize: file.size,
    fileSizeString: this.bytesToString(file.size),
    fileContent: null,
    validContent: file.size > 0 ? undefined : null,
    commaFix: undefined,
    contentInfo: [],
};

• Always use === and the like