lushi/tic_tac_toe — pull request #2 by zachallaun (2013-02-14 05:25:40+00:00)
Title: Code review and suggested changes!

This is getting really good! I tried to make each of my commits rather small so they only change one thing. Hopefully they're easy to follow. :smiley:

### Explanations by commit

**1. Simplify state return value**
The logic here is basically "if s isn't already set to something, check for a tie." Instead of appending `&& s.nil?`, we could have written
```rb
s ||= "tie" if !@board.include? nil
# which is the same as
s || s = "tie" if !@board.include? nil
```
This basically means: if s is falsey (i.e. false or nil), set s to the result of this expression. Of course, we only need to set s at all because it returns later in the function. Since we don't do anything between this and the return, we can drop the assignment altogether, leaving us with
```rb
s || ("tie" if !@board.include? nil)
```
_Note: we have to wrap part of this expression in parentheses so that the `if` doesn't apply to the whole expression._

**2. !foo.nil? => foo**
Basically, asking if something is not false is the same as asking if it's true.

**3. Replace an .each with .find**
Let's quickly look at the first version:
```rb
[4, 0, 2, 6, 8].each do |n|
if @board[n].nil?
@computer_move = n
break
end
end
```
So that we don't iterate through everything if we find an early success, we `break` when we find an empty spot. It turns out that this is a pretty common thing to do, so there is another Enumerable method available to do it for us: `find`.
```rb
@computer_move = [4, 0, 2, 6, 8].find { |n| @board[n].nil? }
```
`find` iterates through some sequence, returning the first item where a boolean expression is truthy. In this case, we're looking for the first empty spot in `@board`, which will be the computer's move.

**4. Unpack triple instead of indexing into it**
This is just a fun thing you can do in Ruby (and some other languages). If you have some sequence of elements, you can "unpack" them into multiple variables:
```rb
x, y = [0, 1]
x #=> 0
y #=> 1
```
Thankfully, this can be done anywhere variables are bound. So when we iterate over winning combinations, we can avoid having to index into them by unpacking the triples:
```rb
@winning_combos.each do |(x, y, z)|
```

**5. Eliminate redundant line**
This basically comes down to preference. The problem is that you can't check if `@board[@computer_move].nil?` before `@computer_move` is set because you'll get a type error. The obvious thing to do is to set it before you do the check. The less obvious thing is to prepend `@computer_move &&` to the expression. Because `&&` is "short-circuiting," the board check won't happen and we can set the random move. The next time through the loop, the computer move will be set and we can check the board.

**6. Make assignment easier to see**
This change is rather simple, using our friend `Enumerable#find` instead of setting the move inside of the loop. This has the added benefit of not looping extra times as well.

**That's all!**
I hope this was helpful! I certainly had fun :smiley:.