HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Using generators to build up a maze

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mazebuildgeneratorsusing

Problem

In prepping to teach a workshop on recursion I wrote this code that uses a maze-building algorithm. In doing so I found it really natural to use generators a lot. I feel pretty happy with how the code turned out but its also pretty unusual and I wonder what yall think of it.

This is meant to run on latest chrome (partial es2015 support, no destructing assignment, no modules yet)




* {
box-sizing: border-box;
}
body {
background-color: red;
min-height: 100vh;
padding: 0;
margin: 0;
display: flex;
}
main {
flex: 1;
display: flex;
flex-direction: column;
}
.row {
display: flex;
flex: 1;
}
.row:first-child .cell {
border-top: 2px solid red;
}
.row .cell:first-child {
border-left: 2px solid red;
}

.cell {
display: flex;
background-color: yellow;
width: 2%;
border-bottom: 2px solid red;
border-right: 2px solid red;
transition: background-color 1.5s;
}
.cell.clear-right {
border-right: 0;
}
.cell.clear-down {
border-bottom: 0;
}
.cell.visited {
background-color: grey;
}
fieldset {
position: fixed;
bottom: 0;
right: 0;
background-color: beige;
max-height: 5px;
transition: max-height .25s;
overflow: hidden;
}
fieldset:hover {
max-height: 3em;
}

Speed:


Finish

(function(){
'use strict'
var speed = document.querySelector('[name=speed]')
var runLater = fn => setTimeout(fn, speed.value)
document.querySelector('[name=finish]').addEventListener('click', () => runLater = fn => fn())

const board = createBoard(50, 50)
const cell = pos(randomInteger(board.width), randomInteger(board.height))
run(createMaze(board, null, cell))

function run(iterator, prev) {
if( (prev||{}).done ) return
runLater( () => run(iterator, iterator.next()))
}

function* createMaze(board, fromCell, toCell) {
// console.log(
moving from ${fromCell} -> ${toCell}`)
board.move(fromCell, toCell)
yield
yield* createMazeOnUnvisitedNeighbors(board, toCell)
}

function* createMazeOnUnvisitedNeighbors(board, cell) {
const surroundingCells = board.surroundingCells(cel

Solution

Indentation

You're missing a level of indentation in your IIFE - it should be:

(function() {
    'use strict'


I've used spaces rather than a tab as I prefer that. I'm not going to recommend one over the other as I don't want to start a holy war.

Spacing

You're inconsistent with spacing around your brackets, sometimes they get a space and sometimes they don't:

board.surroundingCells(cell)


vs

surroundingCells.filter(c => !board.isVisited(c) )


I'd generally never put a space after an opening bracket or before a closing one.

That's the stylistic stuff out the way, I haven't mentioned semicolons because I don't want to start the other JavaScript holy war.

Hoisting

No, hoisting isn't good. It confuses beginners and I don't like having to scroll below the line I'm reading to find the function. Declare your functions first.

var a = true;
function a() {
   return "hello";   
}
console.log(typeof a);


Ask some beginners what the code above logs. I bet at least some of them will get it wrong. Obviously naming a variable the same as a function in the same scope is a pretty big mistake but I have seen it happen.

Confusing code

Look at this bit of code:

var speed = document.querySelector('[name=speed]')
var runLater = fn => setTimeout(fn, speed.value)
document.querySelector('[name=finish]').addEventListener('click', () => runLater = fn => fn())

function run(iterator, prev) {
    if( (prev||{}).done ) return
    runLater( () => run(iterator, iterator.next()))
}


First, speed is a poor name as it is actually a delay in milliseconds. prev is also a poor name, I assume you meant previous but even then, I think current is a better name. It's certainly not the previous step.

I'd move all of the run stuff into a separate object with functions like start, changeDelay and finish.

Recursion

Tail recursion is a special case that is optimised in many languages/compilers to a loop. A loop is far simpler as you don't need to save a new frame on the stack.

function* getRowsHtml(height, width) {
    if(height '
    yield* getCellsHtml(width)
    yield ''
    yield* getRowsHtml(height-1, width)
}


If you refactor that to use a loop you can also only compute the cells' html once instead of having to recompute it for each row (width doesn't change between rows).

I also think all the generator/iterator stuff is pointless for the html as it is all called from getTableHtml which is only called by Array.from.

File outline

I see what you're saying about being able to read the file as an outline, I generally do this sort of thing:

(function (w, $) {
    'use strict';

    var field1,
        field2,
        function1,
        main;

     function1 = function(arg) {
         // Do something important.
     };

     main = function() {
        // entry point.
     };

     main();
}(window, jQuery));

Code Snippets

(function() {
    'use strict'
board.surroundingCells(cell)
surroundingCells.filter(c => !board.isVisited(c) )
var a = true;
function a() {
   return "hello";   
}
console.log(typeof a);
var speed = document.querySelector('[name=speed]')
var runLater = fn => setTimeout(fn, speed.value)
document.querySelector('[name=finish]').addEventListener('click', () => runLater = fn => fn())

function run(iterator, prev) {
    if( (prev||{}).done ) return
    runLater( () => run(iterator, iterator.next()))
}

Context

StackExchange Code Review Q#110994, answer score: 4

Revisions (0)

No revisions yet.