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

Sudoku app for Ruby

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

Problem

I have written a Sudoku app for Ruby. This is my first attempt at coding in Ruby so there are sure to be lots of way to optimise it or make it more efficient.

To summarise the code: it takes an input file of a Sudoku problem in the format comma separated row, column, value of the initial Sudoku problem.

This is stored in an array and a hash. The user can then add or delete numbers with the function adddelete(array,hash) and the program checks whether a repeated number occurs in any box, row or column row with rowcheck(array), colcheck(array) and boxcheck(array). If repeated numbers are found it prints an error message. The game is complete when the Sudoku grid is filled - checkwin(array) with no errors - rowcheck(array), colcheck(array) and boxcheck(array).

Any suggestions on how to improve this would be appreciated!

Some specific questions as no answers yet: What is good practice to test this code once it is written? Profiling? Unit tests? How would I write unit tests for this - just a bunch of Sudoku problems and see if they parse correctly, or would unit tests only be required for something a bit more complicated e.g. a Sudoku solver?

```
def printarray(array)
puts("_________________________")
puts("| " + array[0] + " " + array[1] + " " + array[2] + " | " + array[3] + " " + array[4] + " " + array[5] + " | " + array[6] + " " +
array[7] + " " + array[8] + " | ")
puts("| " + array[9] + " " + array[10] + " " + array[11] + " | " + array[12] + " " + array[13] + " " + array[14] + " | " + array[15] + " " + array[16] + " " + array[17] + " | ")
puts("| " + array[18] + " " + array[19] + " " + array[20] + " | " + array[21] + " " + array[22] + " " + array[23] + " | " + array[24] + " " + array[25] + " " + array[26] + " | ")
puts("|-----------------------|")
puts("| " + array[27] + " " + array[28] + " " + array[29] + " | " + array[30] + " " + array[31] + " " + array[32] + " | " + array[33] + " " + array[34] + " " + array[35] + " | "

Solution

Global variables

The code has a fair number of global variables such as $i which
could possibly be local variables (simply i). Use a local variable
whenever the value does not need to be shared among methods. When it
does need to be shared, prefer a member variable (@i). Global
variables are a source of friction in testing, code reuse, and
refactoring.

Using method names as comments

Good job using comments to help the reader understand the "why" behind
the code. However, a well named method is the best kind of comment.
When you think of writing a comment, consider whether the code you are
commenting coul dbe placed into a separate method, with the name of
that method conveying the same information as the comment. Not all
comments can be removed this way, but many can, and the code will
communicate its intent better.

Use underscores in identifiers

When a method or variable name in Ruby is composed of multiple words,
separate them with underscores, both because it is the common
convention in Ruby, and because it is easier to read. For example,
"add_delete" is preferable to "adddelete."

Make each method do exactly one thing.

Make each method do one thing when possible. This eases the pain of
finding good names, and also makes the code easier to understand.

The method #adddelete is doing three things:

  • Reading the user's move



  • Checking that the move is valid



  • Applying that move to the game board



It may be possible that this function would be made easier to read if
it delegated most of the work to other functions that each did just
one thing.

Consider using two-dimensional arrays

A two-dimensional array is a more natural data structure for
representing a two-dimensional playing board. For example, a
nine-by-nine array initialized with spaces can be constructed like
this:

Array.new(9) { Array.new(9) {" "} }


Case of true/false

The code surprised me with the use of all-caps Boolean literals:

$FAIL = FALSE


I didn't know Ruby had that! However, the literals true and false
should be preferred.

Testing for truthiness

Conditions should rarely test for equivalance with a Boolean literal.
This code:

while $FAIL == TRUE


Would be better as:

while !$FAIL


But consider using "until" to remove the need for the negation:

until $FAIL


Prefer loops with the condition at the top

It often reads easier to put the loop condition at the top of the
loop:

until $FAIL
  ...
end


The syntax is easier to read. Also, when the loop condition is at the
bottom, Ruby will execute the loop body at least once. That might be
what you need, and in fact the reader of your code will expect that
that's why you put the condition at the bottom.

Code Snippets

Array.new(9) { Array.new(9) {" "} }
$FAIL = FALSE
while $FAIL == TRUE
while !$FAIL
until $FAIL

Context

StackExchange Code Review Q#88046, answer score: 3

Revisions (0)

No revisions yet.