Sudoku Game with automatically generated Sudokus

Question

Project I've created a puzzle site with automatically generated sudokus. For more information on Sudokus, please see the wikipedia article. The code const GRID_SIZE = 9; const REMOVE = 50; let solution = createArray(9, 9); let textIsVisible = false; window.addEventListener('load', function() { initialize(); }); document.getElementById('submit').addEventListener('click', function() { let elements = document.getElementsByTagName('input'); let k = 0; for(let i = 0; i < 9; i++) { for(let j = 0; j < 9; j++) { let value = parseInt(elements[k].value); if(value !== solution[i][j]) { textIsVisible = true; $('#text').text('That's not the correct solution!'); return; } k++; } } textIsVisible = true; $('#text').text('Correct!'); }); document.getElementById('reset').addEventListener('click', function() { initialize(); }); function initialize() { textIsVisible = false; $('#text').text(''); let elements = document.getElementsByTagName('input'); for(let i = 0; i < elements.length; i++) { elements[i].classList.add('focus'); elements[i].removeAttribute('readonly'); elements[i].classList.remove('bold'); elements[i].addEventListener('click', function() { $('#text').text(''); }); } let sudoku = generate(); solveSudoku(sudoku, 0); showSudoku(elements, sudoku); } function generate() { //Fill with zeros let sudoku = createArray(9, 9); for(let i = 0; i < 9; i++) { for (let j = 0; j < 9; j++) { sudoku[i][j] = 0; } } //Add ten random numbers for(let i = 0; i < 10; i++) { sudoku = addNumber(sudoku); } /* * Add random numbers in the first row in addition to random numbers * in addition to random numbers on random positions */ sudoku[0][1] = legalNumbers(sudoku, 0, 1); sudoku[0][4] = legalNumbers(sudoku, 0, 4); sudoku[0][7] = legalNumbers(sudoku, 0, 7); //Solve sudoku solveSudoku(sudoku, 0); for(let i = 0; i < 9; i++) { for (let j = 0; j < 9; j++) { sudoku[i][j] = solution[i][j]; } } /* * Remove elements so that sudoku still has unique solution */ let i = 0; while(i < REMOVE) { let row = Math.floor(Math.random() * 9); let col = Math.floor(Math.random() * 9); if(sudoku[row][col] !== 0) { let cache = sudoku[row][col]; sudoku[row][col] = 0; if(solveSudoku(sudoku, 0) !== 1) { /* * Go one step back if sudoku doesn't have unique solution * anymore */ sudoku[row][col] = cache; } else { i++; } } } return sudoku; } function createArray(rows, cols) { const array = new Array(rows); for (let i = 0; i < cols; i++) { array[i] = new Array(cols); } return array; } function addNumber(sudoku) { /* * Find random position to add number */ let row = Math.floor(Math.random() * 9); let col = Math.floor(Math.random() * 9); /* * Add random, but legal number */ sudoku[row][col] = legalNumbers(sudoku, row, col); return sudoku; } function solveSudoku(sudoku, count) { for(let i = 0; i < GRID_SIZE; i++) { for (let j = 0; j < GRID_SIZE; j++) { /* * Only empty fields will be changd */ if(sudoku[i][j] === 0) { /* * Try all numbers between 1 and 9 */ for(let n = 1; n <= GRID_SIZE && count < 2; n++) { /* * Is number n safe? */ if(checkRow(sudoku, i, n) && checkColumn(sudoku, j, n) && checkBox(sudoku, i, j, n)) { sudoku[i][j] = n; let cache = solveSudoku(sudoku, count); /* * new solution found */ if(cache > count) { count = cache; /* * Save new solution */ for(let k = 0; k < GRID_SIZE; k++) { for(let l = 0; l < GRID_SIZE; l++) { if(sudoku[k][l] !== 0) { solution[k][l] = sudoku[k][l]; } } } sudoku[i][j] = 0; } /* * Not a solution, go one step back */ else { sudoku[i][j] = 0; } } } /* * No other solution found */ return count; } } } /* * found another solution */ return count + 1; } function showSudoku(elements, sudoku) { let k = 0; for(let i = 0; i < GRID_SIZE; i++) { for(let j = 0; j < GRID_SIZE; j++) { if(sudoku[i][j] > 0) { elements[k].value = sudoku[i][j]; elements[k].setAttribute('readonly', 'true'); elements[k].classList.remove('focus'); elements[k].classList.add('bold'); } else { elements[k].value = ''; } k++; } } } /* * Helper functions */ function checkRow(sudoku, row, n) { for(let i = 0; i < GRID_SIZE; i++) { if(sudoku[row][i] === n) { return false; } } return true; } function checkColumn(sudoku, col, n) { for(let i = 0; i < GRID_SIZE; i++) { if(sudoku[i][col] === n) { return false; } } return true; } function checkBox(sudoku, row, col, n) { row = row - row % 3; col = col - col % 3; for(let i = row; i < row + 3; i++) { for(let j = col; j < col + 3; j++) { if(sudoku[i][j] === n) { return false; } } } return true; } function legalNumbers(sudoku, row, col) { let array = []; for(let i = 1; i <= 9; i++) { if(checkRow(sudoku, row, i) && checkColumn(sudoku, col, i) && checkBox(sudoku, row, col, i)) { array.push(i); break; } } return array[Math.floor(Math.random() * array.length)]; } function isNumber(elem, e) { if(elem.value.length !== 0) { return false; } let id = e.key; let string = '123456789'; for(let i = 0; i < string.length; i++) { if(string.charAt(i) === id) { return true; } } return false; } Sudoku

Sudoku

Question All suggestions are welcome. The html-file only is a minimal working example, so it is not really worth reviewing.

CertainPerformance · Accepted Answer

Always use const when possible - only use let when you want to warn other readers of the code that the variable name may be reassigned. If something won't be reassigned, make it clear to readers that they don't have to worry about that by using const instead of let.
Anonymous callback wrappers around a single function are superfluous, at least in almost all cases - just pass the single function as the callback instead (see below).
Prefer DOMContentLoaded over load - load waits for all resources to completely finish downloading, including possibly heavyweight images. Better to make the page interactive as soon as the DOM is populated, instead of waiting for sub-resources to finish.
window.addEventListener('DOMContentLoaded', initialize);

Or, since you're using jQuery, you could also do:
$(initialize);

jQuery or vanilla DOM methods? At the moment, you're using jQuery for only one purpose - to call .text, to set the text of elements. This is odd: jQuery is a big library, and element text can be set easily by simply using textContent, no library required. If you do want to make this into a jQuery project (which I wouldn't recommend, since it doesn't seem to be accomplishing anything useful at the moment), it would be more stylistically consistent to use jQuery whenever you're using DOM manipulation.
textIsVisible doesn't do anything You reference it 4 times, but always to assign to it, never to read from it - might as well remove it entirely.
Prefer array methods and iterators over for loops that require manual iteration and have no abstraction. For example, given:
let elements = document.getElementsByTagName('input');
for(let i = 0; i < elements.length; i++) {
    elements[i].classList.add('focus');
    // do lots of other stuff with elements[i]

Unless you need to use the index somewhere other than to reference elements[i], it would make more sense to write DRY and more abstract code with an iterator. It's shorter too:
for (const element of elements) {
    element.classList.add('focus');
    // do lots of other stuff with element

Aleksei's answer shows how to use array methods with other parts of the code too.
Condense class names Since the cells will always have exactly 1 of 2 classes, focus or bold, consider using just a single class instead that you toggle on and off - perhaps focus. For example, instead of:
.bold {
    font-weight: bold;
    font-size: 1em;
}

use
.container > input {
    font-weight: bold;
    font-size: 1em;
}

whose rules get overridden by a .container > .focus rule below.
Don't reassign a variable to the same object you already have a reference to. That is:
sudoku = addNumber(sudoku);

should be
addNumber(sudoku);

Use comments to describe why, or what if what isn't obvious - for example:
// Go one step back if sudoku doesn't have unique solution anymore

is good, whereas
//Solve sudoku
solveSudoku(sudoku, 0);

doesn't communicate anything useful.
solveSudoku is somewhat confusing: it returns a value that often isn't used by callers, and reassigns the solution into a variable completely disconnected from the caller. For example:
let sudoku = generate();
solveSudoku(sudoku, 0);
showSudoku(elements, sudoku);

The purpose of solveSudoku is mysterious until you read the function to find out what its side-effects are. Functions are easiest to make sense of at a glance when they're pure, or at least not too impure. Here, consider returning the value from solveSudoku and using it, eg:
const sudoku = generate();
solution = solveSudoku(sudoku, 0).solution; // with `numUniqueSolutions` as the other property in the object
showSudoku(elements, sudoku);

That makes a lot more sense at a glance. You could go further and avoid the reassignment of the solution variable at all, but that would require some significant refactoring.
The above approach also makes it much clearer what sort of value solveSudoku is returning: numUniqueSolutions (This isn't obvious at a glance from your current implementation of solveSudoku)

Aleksei Tirman · Answer

I suggest writing code in functional style instead of excessive usage of loops. This will make your code more concise (you can reduce code for your solution by half) and readable. I rewrote a few of your functions to give an idea of what I mean:
const range = (from, to) => [...Array(to - from + 1).keys()].map((i) => i + from);

const checkRow = (sudoku, row, n) => range(0, GRID_SIZE - 1).every((x) => sudoku[row][x] !== n);
const checkColumn = (sudoku, col, n) => range(0, GRID_SIZE - 1).every((x) => sudoku[x][col] !== n);

const combine = (arr1, arr2) => arr1.reduce((result, x) => result.concat(arr2.map((y) => [x, y])), []);

const boxCells = (row, col) => {
    row = row - row % 3;
    col = col - col % 3;
    return combine(range(row, row + 2), range(col, col + 2));
}

const checkBox = (sudoku, row, col, n) => boxCells(row, col).every(([r, c]) => sudoku[r][c] !== n);

const legalNumbers = (sudoku, row, col) => {
    const valid = (x) => checkRow(sudoku, row, x) && checkColumn(sudoku, col, x) && checkBox(sudoku, row, col, x);
    return range(1, 9).find(valid);
}

const isNumber = (elem, { key }) => elem.value === '' && '123456789'.split('').includes(key);

Sudoku Game with automatically generated Sudokus

Project

The code

Question

2 Answers

Add your own answers!

Ask a Question