0
\$\begingroup\$

I just solved a problem from leetcode that is faster than 28.42% of the PHP codes, so the main review target is not the performance or alternative ways to solve this problem. I wrote the code thinking about DX, how readable and clear my code is, but currently I don't have anybody to judge my code.

A particularity of the algorithm is that I tried to use hash-maps as much as I could, even though I'm aware that I didn't give too much attention to designing the hash key, to be honest.

I would like to hear your ideas how I could improve the way I name my variables and separate the responsibilities through the functions.

Thanks :)
Also, any other advice is very welcome!

Here is my algorithm:

class Solution {
    
    public function rotateBoard($board) {
        $newBoard = [];

        $boardWidth = $board[0];
        foreach($boardWidth as $column=>$value) {
            $newRow = [];

            foreach($board as $row) {
                $digit = $row[$column];
                array_push($newRow, $digit);
            }

            array_push($newBoard, $newRow);
        }
        
        return $newBoard;
    }
    
    public function squashBox($board) {
        $flatBoxes = [];

        $boxIndexes = [0, 3, 6];
        $boardLength = count($board[0]);
        $boxRows = [];
        for($start = 0; $start < $boardLength; $start += 3) {
            for($row = 0; $row < $boardLength; $row += 1) {
                $boxWidth = 3;
                $boxRow = array_slice($board[$row], $start, $boxWidth);
                $boxRows[] = $boxRow;
            }
        }

        $boxesList = [];
        for($start = 0; $start < count($boxRows); $start += 3) {
            $end = 3;
            $box = array_slice($boxRows, $start, $end);
            $boxesList[] = $box;
        }

        $boxes = [];
        for($boxIndex = 0; $boxIndex < count($boxesList); $boxIndex += 1) {
            $boxList = $boxesList[$boxIndex];
            $row = [];
            for($box = 0; $box < count($boxList); $box += 1) {
                $choosenBox = $boxList[$box];
                for($index = 0; $index < count($choosenBox); $index += 1) {
                    $digit = $choosenBox[$index];
                    array_push($row, $digit);
                }
            }
            array_push($boxes, $row);
        }

        return $boxes;
    }

    public function validateRows($board) {   
        foreach($board as $row) {
        $map = [];

            foreach($row as $digit) {
                if($map[$digit] === true) {
                    return false;
                } elseif($digit !== '.') {
                    $map[$digit] = true;
                }
            }
        }
        return true;    
    }
    
    public function validateColumns($board) {   
        $rotatedBoard = $this->rotateBoard($board);
        return $this->validateRows($rotatedBoard);
    }
    
    public function validateBoxes($board) {
        $flatBoxes = $this->squashBox($board);
        return $this->validateRows($flatBoxes);
    }
    
    function isValidSudoku($board) {
        return $this->validateBoxes($board) && $this->validateColumns($board) && $this->validateRows($board);
    }
}

And here is a input sample:

[["5","3",".",".","7",".",".",".","."],
["6",".",".","1","9","5",".",".","."],
[".","9","8",".",".",".",".","6","."],
["8",".",".",".","6",".",".",".","3"],
["4",".",".","8",".","3",".",".","1"],
["7",".",".",".","2",".",".",".","6"],
[".","6",".",".",".",".","2","8","."],
[".",".",".","4","1","9",".",".","5"],
[".",".",".",".","8",".",".","7","9"]]
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Your rotateBoard() method is merely performing "transposition" upon an indexed array of rows. This is a subject that I have somewhat recently conducted exhaustive research on. Your method is returning an indexed array of indexed arrays, and I am assuming that your input is also an indexed array of indexed arrays. If these assumptions are true, then your entire method can be reduced to a specialized one-liner that knowing developers love to pull out of their toolbelt.

public function transposeBoard(array $board): array
{
    return array_map(null, ...$board); // transposes *multi-row* multidimensional arrays
}

I didn't bother to understand what squashBox() is meant to do, but again, you can lose a WHOOOOOOOOOOOOOOOOOOOOOOOOOOLE lot of code bloat by replacing it with another one-liner that knowing developers love to use for flattening array data. array_merge(...$array) flattens a two-dimensional array. Wrapping it in square braces moves the flat array to be the lone element/row of a multidimesional array. (Demo) If you don't actually need the return value to be two dimensional, remove the square braces.

public function flattenBoard(array $board): array
{
    return [array_merge(...$board)];
}

In validateRows(), I do not support your comparison of $map[$digit] before $map[$digit] is actually declared, this is going to be barfing notices/warnings at you. This gives your code an instantly bad smell. You must check if a variable exists before trying to access it (if it is not guaranteed to exist).

I must say that it was not immediately clear how your validateRows() method was validating. I believe it is checking for duplicates in each row (subarray), but in my experience, this is not enough validation. Perhaps, I need to see the sample input data to understand better.

Couldn't you just return false if a row/subarray was not equal to the return of array_unique(row/subarray)? I mean these arrays are going to be quite small in data volume.


Overall, I'd recommend that you use type declarations for incoming parameters and return values with your methods. If you have methods that will only be called by other methods in this class, make those methods private.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much, some concepts I didn't even knew, like transposition. I've updated the question with a input example if you want to write more. \$\endgroup\$ May 16, 2022 at 20:35

Your Answer

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

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