4
\$\begingroup\$

I recently crafted a binary word search puzzle using HTML, CSS and JavaScript to dynamically build grids within a grid:

var board = document.getElementById('board');
var characters = [
    'H','U','O','M','S','F','I','R','E','I',
    'P','O','B','P','O','S','P','K','K','A',
    'S','E','P','T','E','M','B','E','R','F',
    'Y','T','A','E','W','D','T','I','O','T',
    'W','W','N','S','R','A','I','E','B','E',
    'X','E','H','P','T','A','Y','Z','S','R',
    'S','L','X','G','Y','E','T','U','F','N',
    'K','F','D','R','E','Q','R','I','X','O',
    'Y','T','H','Q','Z','H','X','N','O','O',
    'L','H','Z','D','F','O','U','R','T','N'
];
for (var iCharacters = 0; iCharacters < characters.length; iCharacters++) {
    var binary = '0' + characters[iCharacters].charCodeAt(0).toString(2);
    var square = '<div class="square">';
    for (var iBinary = 0; iBinary < binary.length; iBinary++) {
        if (iBinary == 4) {
            square += '<div class="empty"></div>';
            square += '<div>' + binary.charAt(iBinary) + '</div>';
        }
        else
            square += '<div>' + binary.charAt(iBinary) + '</div>';
    }
    board.innerHTML += square + '</div>';
}
html, body {
    background: linear-gradient(90deg, #00C9FF 0%, #92FE9D 100%);
}
.board {
    width: 800px;
    height: 800px;
    display: grid;
    grid-template-columns: repeat(10, 10%);
    grid-template-rows: repeat(10, 10%);
    grid-gap: 5px;
    border-radius: 5px;
}
.square {
    display: grid;
    grid-template-columns: repeat(3, 33%);
    grid-template-rows: repeat(3, 33%);
    border-radius: 5px;
    background-color: #fff7;
}
.square div {
    display: flex;
    align-content: center;
    justify-content: center;
    font-size: 15px;
    color: #444;
}
.square div:nth-child(1) { border-top-left-radius: 5px; }
.square div:nth-child(3) { border-top-right-radius: 5px; }
.square div:nth-child(7) { border-bottom-left-radius: 5px; }
.square div:nth-child(9) { border-bottom-right-radius: 5px; }
.square .empty {
    background-color: #0003;
    border-radius:5px;
}
<div id="board" class="board"></div>

Since I don't use HTML, CSS and JavaScript as often as I used to, it would be nice to have someone review my code to answer the following questions:

  • Is my code legible?
  • Are there more efficient methods I could have used?

Note: The use of Hungarian notation isn't something I typically do, however in the case where I have multiple iteration variables, I utilize the notation to help denote that the variable I'm working with is, an iteration variable. As such iBinary does not mean that it is an integer, but rather an iteration variable. To me, this is easier to follow when it doesn't make sense to use x,y,z or i,j,k.

\$\endgroup\$

3 Answers 3

Reset to default
3
\$\begingroup\$

Review

You code looks very old school. Use modern JavaScript that simplifies common tasks and helps improve performance.

General points

  • Use constants for variables that do not change. characters, board, binary should all be const
  • Names are a little long. Use common abbreviations when possible. eg for programmers characters is written chars, binary is bin. Loop index's are short iBinary can just be i, idx, or j
  • Use for of when you don't need the indexes.
  • Avoid writing markup to the page. It is very slow compared to using the DOM API. see rewrite
  • Use === (Strict equality) rather than == (Equality). Same for !== (Strict inequality) rather than != (Inequality)
  • Dont repeat code. The two statement blocks inside the loop both end in the same line. Remove the else and the line square += '<div>' + binary.charAt(iBinary) + '</div>'; need only be written once.
  • ALWAYS delimit code blocks. eg never do else foo = 0; always else { foo = 0; } same with if, for, while, etc..

Rewrite

The rewrite does not resemble your code at all as it uses modern JavaScript.

The code is packed into a function as you should never write code outside a function.

;(() => {
    "use strict";
    const tag = (tag, props = {}) => Object.assign(document.createElement(tag), props);
    const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), par);
    const rows = [
        'HUOMSFIREI',
        'POBPOSPKKA',
        'SEPTEMBERF',
        'YTAEWDTIOT',
        'WWNSRAIEBE',
        'XEHPTAYZSR',
        'SLXGYETUFN',
        'KFDREQRIXO',
        'YTHQZHXNOO',
        'LHZDFOURTN'
    ];
    const char2BinArr = c => [...c.charCodeAt(0).toString(2).padStart(8, "0")];
    function addBin(bit, i) {
        const res = [tag("div", {textContent: bit})];
        i === 4 && res.unshift(tag("div", {className: "empty"}));
        return res;
    }
    function createBoard(rows) {
        append(board, 
            ...rows.map(row => [...row].map(char => {
                return append(
                    tag("div", {className: "square"}),
                    ...char2BinArr(char).map(addBin).flat()
                );
            })).flat()
        );
    }
    createBoard(rows);
})();
html, body {
    background: linear-gradient(90deg, #00C9FF 0%, #92FE9D 100%);
}
.board {
    width: 800px;
    height: 800px;
    display: grid;
    grid-template-columns: repeat(10, 10%);
    grid-template-rows: repeat(10, 10%);
    grid-gap: 5px;
    border-radius: 5px;
}
.square {
    display: grid;
    grid-template-columns: repeat(3, 33%);
    grid-template-rows: repeat(3, 33%);
    border-radius: 5px;
    background-color: #fff7;
}
.square div {
    display: flex;
    align-content: center;
    justify-content: center;
    font-size: 15px;
    color: #444;
}
.square div:nth-child(1) { border-top-left-radius: 5px; }
.square div:nth-child(3) { border-top-right-radius: 5px; }
.square div:nth-child(7) { border-bottom-left-radius: 5px; }
.square div:nth-child(9) { border-bottom-right-radius: 5px; }
.square .empty {
    background-color: #0003;
    border-radius:5px;
}
<div id="board" class="board"></div>

References

Reference to features used in the rewrite.

\$\endgroup\$
2
\$\begingroup\$

Answering to your first question your code is certainly legible, I would modify your main structure (I'm copying just the first two rows for brevity):

var characters = ['H','U','O','M','S','F','I','R','E','I',
    'P','O','B','P','O','S','P','K','K','A',
];

You could separate the two rows in your matrix with a clean cut using differents strings for every row and with join and split methods:

const characters = ['HUOMSFIREI','POBPOSPKKA'].join('').split('');

I saw you used only var in your code, this is legitimate but if you have the possibility better use let and const instead of it for a better clarity of your code and to avoid problems related to the scope, see var for more informations.

About your loop:

for (var iCharacters = 0; iCharacters < characters.length; iCharacters++) {
    var binary = '0' + characters[iCharacters].charCodeAt(0).toString(2);
    var square = '<div class="square">';
    for (var iBinary = 0; iBinary < binary.length; iBinary++) {
        if (iBinary == 4) {
            square += '<div class="empty"></div>';
            square += '<div>' + binary.charAt(iBinary) + '</div>';
        }
        else
            square += '<div>' + binary.charAt(iBinary) + '</div>';
    }
    board.innerHTML += square + '</div>';
}

The line square += '<div>' + binary.charAt(iBinary) + '</div>'; is present in both the two branches of your if-else statement, you can rewrite it like below :

if (iBinary === 4) {
   square += '<div class="empty"></div>';
}
square += '<div>' + binary.charAt(iBinary) + '</div>';

You can also use the for...of statement and rewrite your loop in this way:

for (const character of characters) {
    const binary = '0' + character.charCodeAt(0).toString(2);
    let square = '<div class="square">';

    for (let iBinary = 0; iBinary < binary.length; ++iBinary) {
        if (iBinary === 4) {
            square += '<div class="empty"></div>';
        }
        square += '<div>' + binary.charAt(iBinary) + '</div>';
    }
    board += square + '</div>';
}
\$\endgroup\$
2
\$\begingroup\$

For the first question, i.e.

Is my code legible?

It is somewhat legible. I can understand what it is doing. Bearing in mind you stated you don't typically use Hungarian notation I'd point out that the naming could be improved. For e

Have you seen the accepted answer to the SO post Why shouldn't I use "Hungarian Notation"? as well as other answers? The accepted answer points out that often Apps Hungarian is used instead of (non-Apps) Hungarian.

When I see a variable name like iCharacters my first thought might be that it holds the total number of characters in the array. However I can see that it is used as a counter variable incremented from 0 to one less than the total number of characters in the array. A more appropriate name might be something like iCharacterIndex or iCurrentCharacterIndex.

Then variables like board, characters, etc. don't use Hungarian notation (which I prefer). It is better to either exclusively use it or not.

The spacing is consistent with idiomatic JavaScript.

For the second question, i.e.

Are there more efficient methods I could have used?

Just as I mentioned in this answer it is wise to Minimize browser reflows. That means instead of adding elements to the DOM in a loop - e.g. appending to board.innerHTML at the end of the outer for loop, add elements to a documentFragment or temporary element and then add the contents of that element in a single step.

In the past there were discussions about the most efficient way to concatenate strings. Browsers have come a long way in the past decade and that may not really be a major concern anymore, especially since this code likely only runs once when the page loads. Also, Ecmascript 6 template literals can help.

Another option might be using a <template> tag to create elements without an inner loop.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.