Sudoku app for Ruby

Question

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] + " | ")
    puts("| " + array[36] + " " + array[37] + " " + array[38] + " | " + array[39] + " " + array[40] + " " + array[41] + " | " + array[42] + " " + array[43] + " " + array[44] + " | ")
    puts("| " + array[45] + " " + array[46] + " " + array[47] + " | " + array[48] + " " + array[49] + " " + array[50] + " | " + array[51] + " " + array[52] + " " + array[53] + " | ")
    puts("|-----------------------|")
    puts("| " + array[54] + " " + array[55] + " " + array[56] + " | " + array[57] + " " + array[58] + " " + array[59] + " | " + array[60] + " " + array[61] + " " + array[62] + " | ")
    puts("| " + array[63] + " " + array[64] + " " + array[65] + " | " + array[66] + " " + array[67] + " " + array[68] + " | " + array[69] + " " + array[70] + " " + array[71] + " | ")
    puts("| " + array[72] + " " + array[73] + " " + array[74] + " | " + array[75] + " " + array[76] + " " + array[77] + " | " + array[78] + " " + array[79] + " " + array[80] + " | ")
    puts("*************************")
    puts("\n")
    return 0
end

def adddelete(array,h)
# dialogue for adding/deleting numbers from sudoku matrix
    puts("add/delete? add = 1, delete = 2\n?")
    add = gets.chomp
    if add == "1"
        puts ("row (1-9)?")
        row = gets.chomp
        if (row.to_i < 1 or row.to_i > 9) then
            puts ("invalid number try again")
            return 0
        end
        puts ("column (1-9)?")
        column = gets.chomp
        if (column.to_i < 1 or column.to_i > 9) then
            puts ("invalid number try again")
            return 0
        end
        puts ("number (1-9)?")
        number = gets.chomp
        if (number.to_i < 1 or number.to_i > 9) then
            puts ("invalid number try again")
            return 0
        end
        row = row.to_i
        column = column.to_i
        array.fill(number,(row-1)*9+column-1,1)
        h.fill(number,2*((row-1)*9+column)-1,1)
        hash = Hash[*h]
    elsif add == "2"
        puts ("row (1-9)?")
        row = gets.chomp
        if row.to_i < 1 or row.to_i > 9 then
            puts ("invalid number try again")
            return 0
        end
        puts ("column (1-9)?")
        column = gets.chomp
        if column.to_i < 1 or column.to_i > 9 then
            puts ("invalid number try again")
            return 0
        end
        puts ("confirm delete y/n?")
        delete = gets.chomp
        if delete == "y" then
            row = row.to_i
            column = column.to_i
            array.fill(" ",(row-1)*9+column-1,1)
            h.fill("x",2*((row-1)*9+column)-1,1)
            hash = Hash[*h]
        else
            return 0
        end
    else
        puts ("invalid number try again")
        return 0
    end
    return 0
end

def rowcheck(array)
    $rowfail = false
    $i = 0
    $num = 8
#check each row for repeated numbers
    begin
        numbers = array[($i)*9..($i+1)*9-1]
        counts = Hash.new 0
        numbers.each do |number|
            counts[number] += 1
        end
        row = $i + 1
        numbers.uniq.each do |number|
           if counts[number].to_i > 1 and number != " " then
               puts "repeated number " + number.to_s + " in row " + row.to_s + "\n" + "\n"
               $rowfail = true
           end
        end
        $i += 1
    end while $i <= $num
    if $rowfail == false
        return 0
    else 
        return 1
    end
    end

def colcheck(array)
#check each column for repeated numbers
    $colfail = false
    $j = 0
    $num = 8 
    begin
        numbers = []
        numbers << array[$j+0]
        numbers << array[$j+9]
        numbers << array[$j+18]
        numbers << array[$j+27]
        numbers << array[$j+36]
        numbers << array[$j+45]
        numbers << array[$j+54]
        numbers << array[$j+72]
        numbers << array[$j+63]
       # puts numbers
        counts = Hash.new 0

        numbers.each do |number|
            counts[number] += 1
        end
        column = $j + 1
        numbers.uniq.each do |number|
           if counts[number] > 1 and number != " " then
               puts "repeated number " + number.to_s + "in column" + column.to_s + "\n" + "\n"
               $colfail = true
           end
        end

        $j += 1
    end while $j <= $num
    if $colfail == false
        return 0
    else
        return 1
    end
    end

def boxcheck(array)
    $boxfail = false
    $k = 1
    $num = 9
    begin
#check each box
        if $k/3.0 <= 1.0 then
            numbers = []
            numbers << array[($k-1)*3]
            numbers << array[($k-1)*3+1]
            numbers << array[($k-1)*3+2]
            numbers << array[($k-1)*3+9]
            numbers << array[($k-1)*3+10]
            numbers << array[($k-1)*3+11]
            numbers << array[($k-1)*3+18]
            numbers << array[($k-1)*3+19]
            numbers << array[($k-1)*3+20]
        end
        if $k/3.0 > 1.0 and $k/3.0 <= 2.0 then
            numbers = []
            numbers << array[($k-1)*6+3*(3-(($k-1)-3))]
            numbers << array[($k-1)*6+3*(3-(($k-1)-3))+1]
            numbers << array[($k-1)*6+3*(3-(($k-1)-3))+2]
            numbers << array[($k-1)*8+5*(3-($k-1-3))-3]
            numbers << array[($k-1)*8+5*(3-($k-1-3))-2]
            numbers << array[($k-1)*8+5*(3-($k-1-3))-1]
            numbers << array[($k-1)*9+6*(3-($k-1-3))]
            numbers << array[($k-1)*9+6*(3-($k-1-3))+1]
            numbers << array[($k-1)*9+6*(3-($k-1-3))+2]
        end
        if $k/3.0 > 2.0 and $k/3.0 <= 3.0 then
            numbers = []
            numbers << array[($k-1)*7+4*(9-(($k-1)))]
            numbers << array[($k-1)*7+4*(9-(($k-1)))+1]
            numbers << array[($k-1)*7+4*(9-(($k-1)))+2]
            numbers << array[($k-1)*8+5*(9-(($k-1)))]
            numbers << array[($k-1)*8+5*(9-(($k-1)))+1]
            numbers << array[($k-1)*8+5*(9-(($k-1)))+2]
            numbers << array[($k-1)*9+6*(9-(($k-1)))]
            numbers << array[($k-1)*9+6*(9-(($k-1)))+1]
            numbers << array[($k-1)*9+6*(9-(($k-1)))+2]
        end
        counts = Hash.new 0
        numbers.each do |number|
            counts[number] += 1
        end

        numbers.uniq.each do |number|
            if counts[number] > 1 and number != " " then
                puts "repeated number " + number.to_s + " in box" + $k.to_s + "\n" + "\n"
                $boxfail = true
            end
        end

        $k+=1
    end while $k<=$num
    if $boxfail == true
        return 1
    else
        return 0
    end
    end

def checkwin(array)
    $i = 0
    $num = 80
    $check = true
    array.each do |number|
        if number == " " then
            $check = false
        end
    end
    return $check
end

# create array to store sudoku numbers and h to store sudoku numbers 
# h to be converted to hash
array = Array.new(81)
h = Array.new(162)
array.fill(" ")
h.fill("x")


# open input file
File.open("/home/squirrel/coding/easy3.sudoku", "r") do |f|

# fill in numbers
$i=0
$num = 81
f.each_line do |line|
    values = line.split(",")
    row = values[0].to_i
    column = values[1].to_i
    number = values[2].to_s.chomp
    array.fill(number,(row-1)*9+column-1,1)
    h.fill(number,2*((row-1)*9+column)-1,1)
end

# fill in hash keys
while $i < $num
    h.fill($i,2*$i,1)
    $i += 1
end

# create hash
hash = Hash[*h]


$FAIL = TRUE

printarray(array)

begin
    adddelete(array,h) 
    printarray(array)
    if (rowcheck(array) + colcheck(array) + boxcheck(array) == 0) and checkwin(array)==true then 
        $FAIL = FALSE
    end
end while $FAIL == TRUE

puts ("Congratulations! You win")

end

A sample input file is below:

1,2,6
1,4,1
1,6,4
1,8,5
2,3,8
2,4,3
2,6,5
2,7,6
3,1,2
3,9,1
4,1,8
4,4,4
4,6,7
4,9,6
5,3,6
5,7,3
6,1,7
6,4,9
6,6,1
6,9,4
7,1,5
7,9,2
8,3,7
8,4,2
8,6,6
8,7,9
9,2,4
9,4,5
9,6,8
9,8,7

Answer 1

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.