Sudoku game in JavaScript

Question

PHP coder here. Learning JavaScript this week. I made a sudoku game for practice. Features:

• Import puzzles by pasting a text string.
• Imported squares are gray. Your squares are white.
• Program doesn't let you make a clearly illegal move. It will blank your number and color the square red for 2 seconds.
• The sudoku puzzle/game is a class. Easy to add methods and features to it later.
• Room for growth. I may add a "solve" button and a "give hints" check box later. I may add a "you won" message when you win.

Things I'm hoping to gain from this Code Review.

• Let's focus on the JavaScript. Improvements to the JavaScript code's organization, style, choice of functions, variable and function names, etc.
• Feel free to refactor completely if you want.
• Looking for that professional coder perspective so I can build good habits.
• Feel free to suggest next steps/next features.

Fiddle - https://jsfiddle.net/AdmiralAkbar2/z6rv70h4/37/

Screenshot

JavaScript

// TODO: solve button
// TODO: show hints
    // if 8 squares in a 3x3 are known, highlight 9th square
    // if 8 squares in a row are known, highlight 9th square
    // if 8 squares in a column are known, highlight 9th square

"use strict";

class Sudoku {
  constructor() {
        this.board = this.blank_board_array();
    }
    
    blank_board_array() {
        return [
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0],
            [0,0,0,0,0,0,0,0,0]
        ];
    }

  // I can't figure out how to get this working with the "set" keyword, so making a method for now
    set_board(board_string) {
        if ( ! board_string.match(/^\d{81}$/m) ) {
            this.board = this.blank_board_array();
            return;
        }
        
        for ( let row = 0; row <= 8; row++ ) {
            for ( let column = 0; column <= 8; column++ ) {
                this.board[row][column] = board_string.charAt(row*9+column);
            }
        }
        
        /*
        if ( ! this.puzzle_is_valid() ) {
            this.board = this.blank_board_array();
            return;
        }
        */
    }
    
    get_board_array() {
        return this.board;
    }
    
    make_move(row, col, value) {
        this.board[row][col] = value;
    }
    
    is_legal_move(row, col, value) {
        // check for non numbers
      // weird that JS match function doesn't put quotes around regex
        if ( ! value.match(/^[1-9]$/m) ) {
            return false;
        }
        
        // check row
        for ( let i = 0; i <= 8; i++ ) {
            if ( value == this.board[row][i] ) {
                return false;
            }
        }
        
        // check column
        for ( let i = 0; i <= 8; i++ ) {
            if ( value == this.board[i][col] ) {
                return false;
            }
        }
        
        // check 3x3 grid
        let row_offset = Math.floor(row/3)*3;
        let col_offset = Math.floor(col/3)*3;
        for ( let i = 0 + row_offset; i <= 2 + row_offset; i++ ) {
            for ( let j = 0 + col_offset; j <= 2 + col_offset; j++ ) {
                if ( value == this.board[i][j] ) {
                    return false;
                }
            }
        }
        
        return true;
    }
};

let game1 = new Sudoku();
let import_string;
let import_button = document.getElementById('import');
let sudoku_squares = createArray(9,9);

for ( let row = 0; row <= 8; row++ ) {
    for ( let col = 0; col <= 8; col++ ) {
        sudoku_squares[row][col] =  document.getElementsByClassName('sudoku')[0].getElementsByTagName('tbody')[0].getElementsByTagName('tr')[row].getElementsByTagName('td')[col].getElementsByTagName('input')[0];
    }
}

import_button.addEventListener("mouseup", function() {
    import_string = document.getElementsByName("import_string")[0].value;
  game1.set_board(import_string);
    print_sudoku_to_webpage(game1);
});

for ( let row = 0; row <= 8; row++ ) {
    for ( let col = 0; col <= 8; col++ ) {
        sudoku_squares[row][col].addEventListener('input', function(e) {
            e.target.classList.remove("invalid");
            
            if ( ! game1.is_legal_move(row, col, e.target.value) && e.target.value != "" ) {
              e.target.value = "";
                highlight_temporarily(e.target, 2000);
            } else {
                game1.make_move(row, col, e.target.value);
            }
        });
    }
}

function print_sudoku_to_webpage(sudoku_object) {
    let board = sudoku_object.get_board_array();
    clear_webpage_board();
    for ( let row = 0; row <= 8; row++ ) {
        for ( let col = 0; col <= 8; col++ ) {
            if ( board[row][col] != 0 ) {
                let input = sudoku_squares[row][col];
                input.value = board[row][col];
                input.classList.add('imported_square');
            }
        }
    }
}

function clear_webpage_board() {
    for ( let row = 0; row <= 8; row++ ) {
        for ( let col = 0; col <= 8; col++ ) {
            sudoku_squares[row][col].value = "";
            sudoku_squares[row][col].classList.remove('imported_square');
        }
    }
}

// This code is borrowed from another website. Thanks google.
function createArray(length) {
    var arr = new Array(length || 0),
        i = length;

    if (arguments.length > 1) {
        var args = Array.prototype.slice.call(arguments, 1);
        while(i--) arr[length-1 - i] = createArray.apply(this, args);
    }

    return arr;
}

function highlight_temporarily(obj, timeout_in_ms){
   obj.classList.add('invalid');
   setTimeout(function(){
        obj.classList.remove('invalid');
   }, timeout_in_ms);
}

HTML

<!DOCTYPE html>
<html lang="en">
<head>
    <title>Sudoku</title>
</head>
<body>

<div class="import">
    <input type="text" name="import_string" value="080100007000070960026900130000290304960000082502047000013009840097020000600003070" />
    <br />
    <button id="import">Import</button>
    <!--
    <button id="solve">Solve</button>
    <input type="checkbox" value="1" name="hints" /> Show Hints
    -->
</div>

<table class="sudoku">
    <tbody>
    <tr class="1">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="2">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="3 thick_bottom">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="4">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="5">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="6 thick_bottom">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="7">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="8">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    <tr class="9">
        <td class="1"><input type="text" maxlength="1" /></td>
        <td class="2"><input type="text" maxlength="1" /></td>
        <td class="3 thick_right"><input type="text" maxlength="1" /></td>
        <td class="4"><input type="text" maxlength="1" /></td>
        <td class="5"><input type="text" maxlength="1" /></td>
        <td class="6 thick_right"><input type="text" maxlength="1" /></td>
        <td class="7"><input type="text" maxlength="1" /></td>
        <td class="8"><input type="text" maxlength="1" /></td>
        <td class="9"><input type="text" maxlength="1" /></td>
    </tr>
    </tbody>
</table>
</body>
</html>

CSS

body {font-family:sans-serif;}

.import {padding-bottom:0.2em;}
.import input[type=text] {width:630px;}

.valid {background-color:limegreen;}
.invalid {background-color:red;}
.imported_square {background-color:lightgray;}

.sudoku {border:4px solid black; border-collapse: collapse;}
.sudoku tr {padding:0;}
.sudoku td {padding:0; border:1px solid black; width:1em;}
.sudoku input {width:1em; border:0; font-size:25pt; text-align:center;}
.sudoku .thick_right {border-right:4px solid black !important;}
.sudoku .thick_bottom {border-bottom:4px solid black;}

Answer 1

The code is mostly easy to read- indentation seems consistent, though the CSS rules are not separated by new lines - a common convention among many style guides. There are a few comments and many functions/methods are self-documenting but it would be good to comment each function/method to be thorough, especially if you intend to have others utilize the code.

It looks like some ecmascript-6 features are used - e.g. Classes, but more could be used- e.g. the spread syntax, default parameters, destructuring assignment and perhaps arrow functions, etc.

Some variables are actually declared as global - e.g. game1, import_string, etc. - since they are not contained inside a function. That is likely not an issue for a small application like this but in a larger application it would be wise to limit the scopes (e.g. inside an IIFE, DOM-loaded callback, etc. to avoid namespace collisions.

---

The biggest thing I notice that is sub-optimal is this block:

 for ( let row = 0; row <= 8; row++ ) {
   for ( let col = 0; col <= 8; col++ ) {
        sudoku_squares[row][col] =  document.getElementsByClassName('sudoku')[0].getElementsByTagName('tbody')[0].getElementsByTagName('tr')[row].getElementsByTagName('td')[col].getElementsByTagName('input')[0];
    }
}

Not only is that line inside the nested loop excessively long, but it means there are 81 DOM lookups just for the first element with class sudoku, plus DOM lookups for each of the child elements.

”...DOM access is actually pretty costly - I think of it like if I have a bridge - like two pieces of land with a toll bridge, and the JavaScript engine is on one side, and the DOM is on the other, and every time I want to access the DOM from the JavaScript engine, I have to pay that toll”
- John Hrvatin, Microsoft, MIX09, in this talk Building High Performance Web Applications and Sites at 29:38, also cited in the O'Reilly Javascript book by Nicholas C Zakas Pg 36, as well as mentioned in this post

One could simply add an id attribute to the table element and then access the rows property instead of querying for the rows, and access the cells property of each row instead of querying for them. Another simplification would be to use document.querySelectorAll() with a selector like .sudoku input to get a collection of the inputs.

---

Similar to PHP’s foreach loops, for..of loops could be used instead of the for loops to avoid having to manually increment the counter variables.

Instead of blocks like this:

for ( let i = 0; i <= 8; i++ ) {
    if ( value == this.board[row][i] ) {
        return false;
    }
}

simplify it like this:

for ( const cell of this.board[row]) {
    if ( value == cell ) {
        return false;
        }
    }

---

The method blank_board_array could be a static method since it doesn’t need to reference any state of the instance. The same could apply to some of the other helper functions. Additionally, createArray() could be used instead of blank_board_array(). And that could perhaps be simplified by utilizing Array.fill().

---

Instead of using let for all block-scope variables, it is wise to default to using const to avoid accidental re-assignment. Then when you determine re-assignment is necessary use let (e.g. for counters, etc.).

---

The mouse up event handler for the import button starts with this line:

  import_string = document.getElementsByName("import_string")[0].value

It would be simpler to add an id attribute to that element and then use document.getElementById() to reference it- that way there is no need to fetch a collection of elements just to get the first one.

If all elements were wrapped in a <form> tag then that element could be referenced by document.forms[0].import_string but then you would have to ensure the form didn’t get submitted.

Also it would likely be wise to use the click event instead of mouseup - that way if the user preferred using the keyboard to tab through elements it would allow pressing the Enter key to also trigger the event handler.

---

The e.g. the spread syntax could be used to transform this:

var args = Array.prototype.slice.call(arguments, 1);

To this:

const args = [...arguments].slice(1);

Or Array.shift() could be used to take the first argument off the array, eliminating the need to call slice().

Or use destructuring assignment to assign those variables.

function createArray() {
    const [length = 0, ...remainingArgs] = arguments;
    var arr = new Array(length),
    i = length;
    if (remainingArgs.length) {
        while(i--) arr[length-1 - i] = createArray(...remainingArgs);
    }

---

There is one CSS rule containing !important:

.sudoku .thick_right {border-right:4px solid black !important;}

It is best to create a rule that is more specific than any others instead of using !important.

When an important rule is used on a style declaration, this declaration overrides any other declarations. Although technically !important has nothing to do with specificity, it interacts directly with it. Using !important, however, is bad practice and should be avoided because it makes debugging more difficult by breaking the natural cascading in your stylesheets. When two conflicting declarations with the !important rule are applied to the same element, the declaration with a greater specificity will be applied.¹

There happen to be no other border-right rules that need to be overridden so that keyword can be removed here.

---

The table rows and cells have class names like “1”, “2”. I don't see any CSS or JavaScript that utilizes those class names. While "CSS2.1 it is a recommendation, even in CSS3"² class names should not start with a number:

Property names and at-rule names are always identifiers, which have to start with a letter or a hyphen followed by a letter, and then can contain letters, numbers, hyphens, or underscores. You can include any code point at all, even ones that CSS uses in its syntax, by escaping it.³

Answer 2

To add to existing review a few points I noticed:

• You are mixing up under_score (clear_webpage_board) and camelCase naming (createArray). Typically functions in javascript use camelCase (you can see that in all built-in methods that you use, including the borrowed createArray).
• Css on the other hand typically uses naming, where dash is used, so thick-right, not thick_right. Ye it's a bit annoying that every technology has different convention naming.
• I don't like your namings very much. print_sudoku_to_webpage. That method sychronizes your data with html elements, that represent it. I would call it maybe displayBoard or something (since you call it "board" in another function. print feels off, you don't print anything. webpage also (duh, you are on a webpage, we know that). clear_webpage_board again drop webpage, no point.
• How to use your code if I wanted more sudoku boards on one page? It wouldn't work, because you are always looking for first element with class "sudoku". It feels like there should be one class representing Sudoku, independent from HTML (you have that) and then there should be another class, that can display it in UI, handle events and pass those events to game itself. That class can know this "root element" with class "sudoku" and look for rest of pieces inside of it using querySelector as our colleague in other answer mentioned. That would also clean all your cycles and code, that doesn't have function anywhere.
• Consider extracting repeating strings to constants - that will help avoiding bugs. If you make typo mistake in one place but not another, sometimes it's hard to debug. Same goes for numbers - that would also make it easier to generate sudoku with different dimensions.

Answer 3

Let's start with some convention things, in JS we (everyone) write function and variable names as camel case.

I see you have a comment that you tried to use a setter, the issue is that you would then want to use it in two ways. You would want to write this.board = import_string to call the setter, but in the constructor you do

this.board = this.blank_board_array();

You could use a setter with some rewriting, but I think it's fine as is.

The double equals in JS == coerces the types, so that for example 0==[] is true, while this can be useful, in my experience it fits squarely in the realm of being too clever, or of things working coincidentally. I would advise you always use === which does not coerce types (but controversially to my colleagues I write lines like if (x === false), some say this is a step too far).

I would suggest that it would be nicer to have functions which create an html Sudoku board, i.e. rather than target elements that happen to be there, you can make the elements with JS and then add a single element (with many children) at the end, if you do this you can also tidy up the logic of setting up the event listeners a lot too.

In general there's been a trend (at least in my experience working) towards writing quite functional code in JS. Your stuff is far removed from that, not really a problem, just something to be aware of.

It's a bit hard to give an example without just rewriting everything, but for instance (though the example I will give is still not very functional) several times you loop over a 2D array, you could write a function loopOverMatrix(matrix, f) which takes a 2D array and a function f (i, j) => {...}, and loops over all the elements [i][j] and calls f(i)(j).