patternrubyMinor
Ruby sudoku solver using backtracking (interview)
Viewed 0 times
interviewusingrubysolversudokubacktracking
Problem
I was required to implement the code to a spec file I was provided in Ruby. I was rejected with the message "The quality of your code isn't sufficient for this position". Can you please tell me what can be improved in order to achieve higher quality?
```
require './game'
require './game_board'
describe GameBoard do
it "should correctly solve the board" do
@game = Game.new
# Each '0' is a blank cell
@game.load_board 0, 0, 8, 3, 4, 2, 9, 0, 0,
0, 0, 9, 0, 0, 0, 7, 0, 0,
4, 0, 0, 0, 0, 0, 0, 0, 3,
0, 0, 6, 4, 7, 3, 2, 0, 0,
0, 3, 0, 0, 0, 0, 0, 1, 0,
0, 0, 2, 8, 5, 1, 6, 0, 0,
7, 0, 0, 0, 0, 0, 0, 0, 8,
0, 0, 4, 0, 0, 0, 1, 0, 0,
0, 0, 3, 6, 9, 7, 5, 0, 0
@solved_board = GameBoard.new 6, 7, 8, 3, 4, 2, 9, 5, 1,
3, 2, 9, 1, 8, 5, 7, 6, 4,
4, 5, 1, 7, 6, 9, 8, 2, 3,
5, 1, 6, 4, 7, 3, 2, 8, 9,
8, 3, 7, 9, 2, 6, 4, 1, 5,
9, 4, 2, 8, 5, 1, 6, 3, 7,
7, 6, 5, 2, 1, 4, 3, 9, 8,
2, 9, 4, 5, 3, 8, 1, 7, 6,
1, 8, 3, 6, 9, 7, 5, 4, 2
@game.solve.should == @solved_board
end
it "should correctly solve the board" do
@game = Game.new
# Each '0' is a blank cell
@game.load_board 0, 0, 4, 0, 0, 0, 5, 0, 0,
0, 7, 0, 2, 0, 0, 3, 6, 0,
8, 0, 0, 0, 0, 1, 0, 0, 0,
6, 2, 9, 0, 0, 0, 0, 3, 0,
0, 0, 0, 0, 6, 0, 0, 0, 0,
0, 4, 0, 0, 0, 0, 6, 1, 8,
0, 0, 0, 7, 0, 0, 0, 0, 6,
0, 1, 3, 0, 0, 4, 0, 2, 0,
0, 0, 2, 0, 0, 0, 4,
game_board_spec.rb (file provided)```
require './game'
require './game_board'
describe GameBoard do
it "should correctly solve the board" do
@game = Game.new
# Each '0' is a blank cell
@game.load_board 0, 0, 8, 3, 4, 2, 9, 0, 0,
0, 0, 9, 0, 0, 0, 7, 0, 0,
4, 0, 0, 0, 0, 0, 0, 0, 3,
0, 0, 6, 4, 7, 3, 2, 0, 0,
0, 3, 0, 0, 0, 0, 0, 1, 0,
0, 0, 2, 8, 5, 1, 6, 0, 0,
7, 0, 0, 0, 0, 0, 0, 0, 8,
0, 0, 4, 0, 0, 0, 1, 0, 0,
0, 0, 3, 6, 9, 7, 5, 0, 0
@solved_board = GameBoard.new 6, 7, 8, 3, 4, 2, 9, 5, 1,
3, 2, 9, 1, 8, 5, 7, 6, 4,
4, 5, 1, 7, 6, 9, 8, 2, 3,
5, 1, 6, 4, 7, 3, 2, 8, 9,
8, 3, 7, 9, 2, 6, 4, 1, 5,
9, 4, 2, 8, 5, 1, 6, 3, 7,
7, 6, 5, 2, 1, 4, 3, 9, 8,
2, 9, 4, 5, 3, 8, 1, 7, 6,
1, 8, 3, 6, 9, 7, 5, 4, 2
@game.solve.should == @solved_board
end
it "should correctly solve the board" do
@game = Game.new
# Each '0' is a blank cell
@game.load_board 0, 0, 4, 0, 0, 0, 5, 0, 0,
0, 7, 0, 2, 0, 0, 3, 6, 0,
8, 0, 0, 0, 0, 1, 0, 0, 0,
6, 2, 9, 0, 0, 0, 0, 3, 0,
0, 0, 0, 0, 6, 0, 0, 0, 0,
0, 4, 0, 0, 0, 0, 6, 1, 8,
0, 0, 0, 7, 0, 0, 0, 0, 6,
0, 1, 3, 0, 0, 4, 0, 2, 0,
0, 0, 2, 0, 0, 0, 4,
Solution
I'm sorry to hear that you didn't get the position. I'm not sure what position you were applying for but this code is certainly as good as most code I get from candidates. I can make some comments but I'm nit picking, I would consider this code fine in production where people have jobs to do and don't have the time to make everything perfect.
-
Maybe add a spec for a failing or unsolvable board (either because the board is invalid or not enough values were provided)
-
I would prefer to see a constant for the board size.
-
This is slightly personal, but I don't like passing
-
You continually refer to
-
If a choice is available in a row and column (which is almost guaranteed) then there will be duplicate elements in this code below
-
For
-
The
-
Maybe add a spec for a failing or unsolvable board (either because the board is invalid or not enough values were provided)
-
I would prefer to see a constant for the board size.
-
This is slightly personal, but I don't like passing
[x,y] as an array and then using first and last to get the values. I would prefer to see a struct, hash or something similar.-
You continually refer to
gameboard.board, I wouldn't make the internal @board public. Instead I would move some if not most logic from Game to GameBoard including get_block, available_block_choices, is_solved? and update_board. I would actually make the first two private and add available_pos_choices and empty? methods so the main code might look like:board.each_with_index do |val, row, col|
next unless board.empty?(i,j)
available_choices = board.available_choices(row, col)
available_choices.each do |choice|
updated_board = board.update(row, col, choice)
return true if solve_game(updated_board)
end
return false
end # each-
If a choice is available in a row and column (which is almost guaranteed) then there will be duplicate elements in this code below
available_pos_choices = available_row_choices &
available_col_choices &
available_block_choices(board, [i,j])-
For
update_board you should probably use the Matrix.clone method.-
The
select here number_set - block.to_a.flatten.select{|e| e != 0} seems redundant. At least you didn't exclude zero in other places.Code Snippets
board.each_with_index do |val, row, col|
next unless board.empty?(i,j)
available_choices = board.available_choices(row, col)
available_choices.each do |choice|
updated_board = board.update(row, col, choice)
return true if solve_game(updated_board)
end
return false
end # eachavailable_pos_choices = available_row_choices &
available_col_choices &
available_block_choices(board, [i,j])Context
StackExchange Code Review Q#158546, answer score: 2
Revisions (0)
No revisions yet.