My version of Tic-Tac-Toe

[Null]

This has nothing to do with the previous posting of a tic-tac-toe game, it's just that I had the same idea.

All the classes (in tictactoe/lib) are finished, but the main program (tictactoe/bin/main.rb) is more of a test game. It is still playable, but it is not intended to be the final game.

I am still relatively new to Ruby, so comments from those more knowledgeable then myself would be welcome.

[wtd]

General advice before even looking at the code. You print a version of the board to show where Xs and Os are. Just combine this with the board representation that contains numbers.

[wtd]

First code suggestion:

Don't mess with $:. Instead you can just:

code:

require "../lib/tictac/board"

Next:

code:

if not board.winner.nil?

Instead of "if not", you can simply use "unless".

Next:

code:

Integer(gets.chomp)

Instead:

code:

gets.chomp.to_i

Next:

code:

puts "\n", board.display

Instead make use of string interpolation:

code:

puts "\n#{board.display}"

Further, your board could have defined its own to_s method, and then this would be:

code:

puts "\n#{board}"

Or:

code:

puts puts board

Next:

I notice you have a fair number of occurences of:

code:

players[player]

This makes sense. You have "player" which is a key for the "players" hash, and using that key you look up the corresponding player.

Why not let "each" do this for you?

code:

players.each do |player_name, player|

Next:

This:

code:

while true

Is a cludge. Just use the loop method.

code:

loop do    # ... end

Next, in board.rb:

code:

class InvalidSpaceError < ArgumentError                         def initialize(msg)                                 super(msg)                         end                 end

You explicitly pass msg to super. You do not need to do this. If you simply call "super", it receives all the same arguments sent to the child class initialize method.

Plus, if you're just sending the same message to the superclass initialize method, then you're not really changing anything. Don't bother changing initialize at all.

Next:

code:

@spaces = Array.new(9, ' ')

When you call Array#new like this, you are setting all nine elements to the same String object. Use a block, and it will set them to unique String objects.

code:

Array.new(9) { ' ' }

Perhaps better yet, represent empty spaces as nil.

Next!

code:

return @winner if not @winner.nil?

Instead:

code:

return @winner unless @winner.nil?

Next:

code:

%w(X O).each do |player|                                 [check_diagonal(player), check_col(player), check_row(player)].each do |status|                                         if not status.nil?                                                 return status                                         end                                 end                         end

The way you have things here, each check is being performed immediately, regardless of the success of the first.

One way around this is to use message passing.

code:

%w(X O).each do |player|                                 [:diagonal, :col, :row].each do |type_of_check|                                         status = send(:"check_#{type_of_check}", player)                                         return status unless status.nil?                                 end                         end

The other option:

code:

%w(X O).each do |player|                                 status = check_diagonal(player) or check_col(player) or check_row(player)                                 return status if status                         end

I think I'll let you chew on these suggestions for awhile.

[Null]

wtd wrote:

First code suggestion:

Don't mess with $:. Instead you can just:

code:

require "../lib/tictac/board"

I am pretty sure that the $:.unshift stuff allows me to run the program from any directory. If I use what you suggested, I can only run the program from inside tictac.

Quote:

code:

if not board.winner.nil?

Instead of "if not", you can simply use "unless".

Thanks, I forgot about unless.

Quote:

code:

Integer(gets.chomp)

Instead:

code:

gets.chomp.to_i

Integer throws an ArgumentError on bad input, to_i just returns 0.

Quote:

code:

puts "\n", board.display

Instead make use of string interpolation:

code:

puts "\n#{board.display}"

Simple change, but thanks regardless

Quote:

Further, your board could have defined its own to_s method, and then this would be:

code:

puts "\n#{board}"

Or:

code:

puts puts board

I have defined Board#to_s. It will display the board filled with X's and O's. Board#display calls to_s and in addition displays a board filled with numbered spots.

Quote:

I notice you have a fair number of occurences of:

code:

players[player]

This makes sense. You have "player" which is a key for the "players" hash, and using that key you look up the corresponding player.

Why not let "each" do this for you?

code:

players.each do |player_name, player|

Good idea. Thanks

Quote:

This:

code:

while true

Is a cludge. Just use the loop method.

code:

loop do    # ... end

Also good to know

Quote:

Next, in board.rb:

code:

class InvalidSpaceError < ArgumentError                         def initialize(msg)                                 super(msg)                         end                 end

You explicitly pass msg to super. You do not need to do this. If you simply call "super", it receives all the same arguments sent to the child class initialize method.

Plus, if you're just sending the same message to the superclass initialize method, then you're not really changing anything. Don't bother changing initialize at all.

I didn't know that.

Quote:

code:

@spaces = Array.new(9, ' ')

When you call Array#new like this, you are setting all nine elements to the same String object. Use a block, and it will set them to unique String objects.

code:

Array.new(9) { ' ' }

Perhaps better yet, represent empty spaces as nil.

I also didn't know that.


Thanks for the useful suggestions, wtd

[wtd]

Null wrote:

Integer throws an ArgumentError on bad input, to_i just returns 0.

Ah, but the error Integer throws will not look nice in your program. You care only to tell the user if the space number is invalid, or already used.

Use "gets.chomp.to_i". If there is an error, zero will be passed, and an InvalidSpaceError will be thrown.

[rdrake]

Null wrote:

wtd wrote:

First code suggestion:

Don't mess with $:. Instead you can just:

code:

require "../lib/tictac/board"

I am pretty sure that the $:.unshift stuff allows me to run the program from any directory. If I use what you suggested, I can only run the program from inside tictac.

../ is relative, it can be used in any directory.

[Null]

cartoon_shark wrote:

Null wrote:

wtd wrote:

First code suggestion:

Don't mess with $:. Instead you can just:

code:

require "../lib/tictac/board"

I am pretty sure that the $:.unshift stuff allows me to run the program from any directory. If I use what you suggested, I can only run the program from inside tictac.

../ is relative, it can be used in any directory.

code:

#$:.unshift(File.join(File.dirname(__FILE__), "..", "lib")) #require 'tictac/board' #require 'tictac/player' require '../lib/tictac/board' require '../lib/tictac/player'

code:

jesse@ubuntu:~/Programs/Ruby/tictactoe$ ruby bin/main.rb bin/main.rb:8:in `require': no such file to load -- ../lib/tictac/board (LoadError)         from bin/main.rb:8

And I am proven correct.


wtd : I like that solution of using the fail-value of 0 to have TicTac::Board::InvalidSpaceError raised. Thanks

[wtd]

Null wrote:

wtd : I like that solution of using the fail-value of 0 to have TicTac::Board::InvalidSpaceError raised. Thanks

Glad I could help.

A huge part of programming is designing your app and defining exactly what proper user input is. The rest of the program is relatively predictable, but humans... humans are chaotic.

[rdrake]

Null wrote:

I am proven correct.

Oh, don't be so sure.

code:

%ruby bin/main.rb bin/main.rb:7:in `require': no such file to load -- ../lib/tictac/board (LoadError)         from bin/main.rb:7 %cd bin %ruby main.rb -----------    |   | -----------    |   | -----------    |   | ----------- -----------  1 | 2 | 3 -----------  4 | 5 | 6 -----------  7 | 8 | 9 ----------- Player 1 <X>: Make move >>>

Like I said, it's relative. Therefore, it'll go down the directory from where the script is run, then change to lib and find the script. There, I win .

[Null]

cartoon_shark wrote:

Null wrote:

I am proven correct.

Oh, don't be so sure.

code:

%ruby bin/main.rb bin/main.rb:7:in `require': no such file to load -- ../lib/tictac/board (LoadError)         from bin/main.rb:7 %cd bin %ruby main.rb -----------    |   | -----------    |   | -----------    |   | ----------- -----------  1 | 2 | 3 -----------  4 | 5 | 6 -----------  7 | 8 | 9 ----------- Player 1 <X>: Make move >>>

Like I said, it's relative. Therefore, it'll go down the directory from where the script is run, then change to lib and find the script. There, I win .

I'm hoping you're joking, as that is exactly my point. You have to be in the correct directory in order to run it.

With my solution (which I took straight out of the pickaxe book), you can run the program correctly from any directory.

[wtd]

You may wish to use $LOAD_PATH instead of $:, as it conveys more information.