-----------------------------------

Null

Sun May 28, 2006 5:10 pm


My version of Tic-Tac-Toe

-----------------------------------

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

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

Sun May 28, 2006 5:30 pm



-----------------------------------

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

Sun May 28, 2006 5:33 pm



-----------------------------------

First code suggestion:

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

require "../lib/tictac/board"

Next:

if not board.winner.nil?

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

Next:

Integer(gets.chomp)

Instead:

gets.chomp.to_i

Next:

puts "\n", board.display

Instead make use of string interpolation:

puts "\n#{board.display}"

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

puts "\n#{board}"

Or:

puts
puts board

Next:

I notice you have a fair number of occurences of:

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?

players.each do |player_name, player|

Next:

This:

while true

Is a cludge.  Just use the loop method.

loop do 
   # ...
end

Next, in board.rb:

		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:

@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.

Array.new(9) { ' ' }

Perhaps better yet, represent empty spaces as nil.

Next!

return @winner if not @winner.nil?

Instead:

return @winner unless @winner.nil?

Next:

		%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.

		%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:

		%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

Sun May 28, 2006 6:09 pm



-----------------------------------

First code suggestion:

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

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.



if not board.winner.nil?

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


Thanks, I forgot about unless. :)



Integer(gets.chomp)

Instead:

gets.chomp.to_i


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


puts "\n", board.display

Instead make use of string interpolation:

puts "\n#{board.display}"


Simple change, but thanks regardless :)


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

puts "\n#{board}"

Or:

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.


I notice you have a fair number of occurences of:

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?

players.each do |player_name, player|


Good idea. Thanks :)


This:

while true

Is a cludge.  Just use the loop method.

loop do 
   # ...
end


Also good to know :)



Next, in board.rb:

		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. :)



@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.

Array.new(9) { ' ' }

Perhaps better yet, represent empty spaces as nil.

I also didn't know that.


Thanks for the useful suggestions, wtd :)


-----------------------------------

wtd

Sun May 28, 2006 6:20 pm



-----------------------------------

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

Sun May 28, 2006 7:30 pm



-----------------------------------

First code suggestion:

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

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

Sun May 28, 2006 8:31 pm



-----------------------------------

First code suggestion:

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

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.


#$:.unshift(File.join(File.dirname(__FILE__), "..", "lib"))

#require 'tictac/board'
#require 'tictac/player'

require '../lib/tictac/board'
require '../lib/tictac/player'



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

Sun May 28, 2006 9:35 pm



-----------------------------------

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

Sun May 28, 2006 9:46 pm



-----------------------------------

I am proven correct. ;)Oh, don't be so sure.
%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 : 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

Sun May 28, 2006 10:05 pm



-----------------------------------

I am proven correct. ;)Oh, don't be so sure.
%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 : 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

Sun May 28, 2006 10:08 pm



-----------------------------------

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