eakray: - Github Plus

pull request opened

Puzzle mapper refactor

Here's a small refactor of the newPuzzle endpoint. Most of the changes are in the puzzleMapper module. The functions can be read from bottom to top, which each enclosed function being defined directly above the caller.

Some thoughts:

The existing two functions were way too big. That's a good heuristic to know when to refactor.
When you're reading the code does it tell a story? And how difficult is it to read that story?
Another heuristic: when you're writing code, always think about how easy it is to read and how clearly you're communicating your intentions, rather than just getting the code to work. It will be read many more times than it will be written.
You should try to avoid using break or continue as that's essentially a GOTO and there are better ways achieve the same results.
In fact, I think a good default is to use iterators instead of looping, i.e. map, filter, reduce, etc. Doing so naturally bundles up the iteration logic in a callback function that can be extracted and tested in isolation and reapplied, instead of dumping a bunch of logic inside the body of the loop. Also, you can think of applying that function as an invariant over the entire iteration. You don't have to worry about setting the start and end variables and conditionally applying the logic.

I don't think this is the best implementation but it's pretty good. We map over the letters to create an array of cell objects (which all have across and down default objects) and then we iterate over that array once more and each time we come across a gridnum we invoke a function that returns an array of indices for all the cells belonging to that clue (in either direction, we specify the direction as an argument when we invoke that function). Then we iterate that many steps and fill out the across/down object.

Another issue with this data model that I haven't addressed in the PR is the unnecessary nesting. It makes the invocation of the scanClueCells function a bit clunky. It would be better if the data model looked something like this:

const puzzleMapper = (puzzle) => ({
    clueArrayAcross: puzzle.clues.across,
    clueArrayDown: puzzle.clues.down,
    acrossClueMap: clueMapper(puzzle.clues.across),
    downClueMap: clueMapper(puzzle.clues.down)
    cells: generateCellsObjectArray(     // disregard this function call
        puzzle.grid,
        puzzle.gridnums,
        puzzle.size,
        clueMapper(puzzle.clues.across),
        clueMapper(puzzle.clues.down),
        puzzle.answers.across,
        puzzle.clues.across,
        puzzle.answers.down,
        puzzle.clues.down
    )
});

// and cells should look like this

    const cells = grid.map((cell, index) => ({
        index,
        gridnums: gridnums[index],
        letter: cell,
        focus: false,
        guess: "",
        row: row(index),
        column: column(index),
        acrossmember: null,
        downmember: null,   
        acrossStart: null,
        downStart: null,
        acrossClue: null,
        downClue: null,
        acrossAnswer: null,
        downAnswer: null,
        acrossPrev: null,
        downPrev: null,
        acrossNext: null
        downNext: null
    }));

Move the contents of acrossMember and downMember to the top level and get rid of focusRange. We already have that information in the othe fields. On the front end, when you click on a clue or a cell, we can modify the object that keeps track of our focus to the current direction and the associated clue along that direction (from the clicked on cell).

Another thing. We probably want to not include the answer data in each cell (or in the payload at all), as we'll make a call to the backend to validate a cell, word, or puzzle.

Created at 3 days ago

push

eakray/Crosswords