Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

At first, I created an unbeatable Tic-Tac-Toe in Python. However, I wanted a better interface, so I used a PHP backend that runs the script and a simple JavaScript that sends user commands to the script.

index.html: (excuse stupid class names)

<html>
<head>
  <script src='jquery.js'></script>
  <link rel="shortcut icon" type="image/png" href="/favicon.png"/>
  <link href='https://fonts.googleapis.com/css?family=Kaushan+Script' rel='stylesheet' type='text/css'>
  <link href='styling.css' rel='stylesheet' type='text/css'>
  <script src='code/ttt-realhard.js'></script>
  <title> Tic-Tac-Toe Unbeatable </title>
  <style>
  table {
    text-align: center;
    vertical-align: middle;
    font: bold 36px sans-serif;
}
p,
h1,
td,
th {
    font-family: 'Lato', sans-serif;
}
table,
td,
th {
    border: none;
    border-collapse: collapse;
  }
    td, th {
        text-align: center;
        font-weight: 300;
        height: 150px;
        width: 150px;
        font-size: 1em;
    }
    #board tr td:hover {
        background: #e4e4e4;
        cursor: pointer;
    }
    .ab_c {
        position: absolute;
        top: 50%;
        left: 50%;
        transform: translate(-50%, -50%);
    }
    td#c0,
    td#c1,
    td#c2,
    td#c3,
    td#c4,
    td#c5 {
        border-bottom: 3px solid grey;
    }
    td#c0,
    td#c1,
    td#c3,
    td#c4,
    td#c6,
    td#c7 {
        border-right: 3px solid grey;
    }
    .lolyoucantbeatme {
        color: grey;
        font-weight: 400;
        font-size: 4em;
    }
    .lolyoureallywontbutyoucantry {
        color: grey;
        font-weight: 100;
        font-size: 2em;
    }
  </style>
</head>
<body>
  <!-- v1.0 unbeatable-->
  <!-- by joseph -->
  <div id='youwontbeatme'>
    <h1 class='lolyoucantbeatme ab_c' style='top:2%'> Tic Tac Toe </h1>
    <p class='lolyoureallywontbutyoucantry ab_c' style='top:10%'> Try to beat me! </p>
  </div>
  <img src='spinner2.gif' id='spinner' width='150' height='150' style='display:none;' class='ab_c'>
  <table id='board' class='ab_c' style='top:55%'>
    <tr>
      <td id='c0' onclick='ttt.ticclick(this)'></td><td id='c1' onclick='ttt.ticclick(this)'></td><td id='c2' onclick='ttt.ticclick(this)'></td>
    </tr>
    <tr>
      <td id='c3' onclick='ttt.ticclick(this)'></td><td id='c4' onclick='ttt.ticclick(this)'></td><td id='c5' onclick='ttt.ticclick(this)'></td>
    </tr>
    <tr>
      <td id='c6' onclick='ttt.ticclick(this)'></td><td id='c7' onclick='ttt.ticclick(this)'></td><td id='c8' onclick='ttt.ticclick(this)'></td>
    </tr>
  </table>
  <div id='lawl'class="ab_c" style="left: 90%;white-space: nowrap;display: none;"><p id='status'></p></div>
</body>
</html>

ttt-realhard.js:

function getRandomInt (min, max) {
    return Math.floor(Math.random() * (max - min + 1)) + min;
}
ttt = {
  "ticclick": function(cell) {
    if ($(cell).html() == '' && !ttt.system.laptopresponse) { $(cell).html('X'); ttt.system.lock();ttt.system.freecode();ttt.system.help(); }
    // TODO: Wait for computer move.
  },
  "realz": [
    '#c0','#c1','#c2',
    '#c3','#c4','#c5',
    '#c6','#c7','#c8'
  ],
}
ttt['system'] = {
  "freecode": function() {
    for (var i = 0; i < ttt.realz.length; i++) {
      var ab = $(ttt.realz[i]);
      if (ab.html() == "X") {
        ab.css('background-color', '#b3b3ff');
      } else if (ab.html() == 'O') {
        ab.css('background-color', '#ffcccc');
      }
    }
  },
  "coolstatus": function(logic, step) {
    if (!logic) return; // stupid hack, too lazy to figure out why sometimes logic is not passed.
    var step = step || 0;
    if (step > logic.length) {$('#lawl').hide();return;}
    $('#lawl').show().html(logic[step]);
    step++;
    setTimeout(function() { ttt.system.coolstatus(logic, step); }, 50)
  },
  "boardencode": function(board) {
    /* Encodes a board to a format that is easily transferred. */
    var buffer = "";
    for (var i = 0; i < board.length; i++) {
      buffer += (board[i] == '' ? '-' : board[i]); // ternary operators are cool
    }
    return buffer;
  },
  "laptopresponse": false,
  "lock": function() {
    console.log('locked');
    $('#spinner').show();
    $('#youwontbeatme').hide();
    ttt.system.laptopresponse = true;
  },
  "unlock": function() {
    console.log('unlocked');
    $('#spinner').hide();
    $('#youwontbeatme').show();
    ttt.system.laptopresponse = false;
  },
  "isWaitingForLaptop": function() {
    return ttt.system.laptopresponse;
  },
  "parseServerResponse": function(dat) {
    // Parse board.
    ab = JSON.parse(dat);
    ttt.system.coolstatus(ab.Board.Logic);
    for (var i = 0; i < ab.Board.DBoard.length; i++) {
      $("#c" + i.toString()).html(null == ab.Board.DBoard[i] ? "" : ab.Board.DBoard[i]);
    }
    // Now, check for a winner.
    ttt.system.freecode();
    if (ab.Board.Complete) {
      // Someone won...
      // Check for a tie, perhaps?
      if (!ab.Board.Winner) {
        // Tie!
        console.log('Tie.');
        $('#spinner').css('width', '200px').css('height','200px');
        $('#youwontbeatme').show().html("<h1 class='lolyoucantbeatme ab_c' style='top:2%'> TIE! </h1>");
        $('.lolyoucantbeatme').css('color', 'orange');
        setTimeout(function(){window.location.replace("index.html")},5e3);
        //$('#lawl').show().html('<a href="easy/"> Play the easy version instead? </a>');
        return 0;
      } else if (ab.Board.Winner == 'O') {
        console.log('Computer win.');
        $('#youwontbeatme').show();
        $('#youwontbeatme').html("<h1 class='lolyoucantbeatme ab_c' style='top:2%'> I won. </h1>");
        $('.lolyoucantbeatme').css('color', 'red');
        $('#c'+ab.Board.WinningCombo[0].toString()).css('font-weight','bold').css('color','red');
        $('#c'+ab.Board.WinningCombo[1].toString()).css('font-weight','bold').css('color','red');
        $('#c'+ab.Board.WinningCombo[2].toString()).css('font-weight','bold').css('color','red');
        setTimeout(function(){window.location.replace("index.html")},5e3);
        //$('#lawl').show().html('<a href="easy/"> Play the easy version instead? </a>');
        return 0;
      }
      alert('Dont cheat bro.');
    }
    // No one won. Unlock board.
    console.log('unlocking board cuz no one won');
    ttt.system.unlock();
  },
  "help": function() {
    console.log('Sending to server: ' + ttt.system.boardencode(ttt.system.collect()))
    // Help!
    pre='inhumane';
    jQuery.get("code/server.php?mode="+pre+"&data=" + ttt.system.boardencode(ttt.system.collect()), ttt.system.parseServerResponse);
  },
  "collect": function() {
    var buffer = [];
    for (var i = 0; i < ttt.realz.length; i++) {
      buffer[i] = $(ttt.realz[i]).html();
    }
    return buffer;
  }
}
var goFirst = Math.floor((Math.random() * 2) + 1);
if (goFirst == 1) {
  $(document).ready(ttt.system.help);
}

server.php:

<?php
  isset($_GET['mode']) || die('ERR_NOMODE');
  isset($_GET['data']) || die('ERR_NODATA');
  $MODE      = $_GET['mode'];
  $BOARDDATA = $_GET['data'];
  $INVOKE    '/Users/joe/Desktop/ScienceFair/ttt/unbeatable_raw.py %s';
  $RESULT    = exec(sprintf($INVOKE, $BOARDDATA));
  // echo('Invoked '. sprintf($INVOKE, $BOARDDATA));
  echo($RESULT);
?>

And finally, the guts (python script):

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import random, os, sys, json

class Tic(object):
    lastwincombo = []
    logic = []
    winning_combos = (
        [0, 1, 2], [3, 4, 5], [6, 7, 8],
        [0, 3, 6], [1, 4, 7], [2, 5, 8],
        [0, 4, 8], [2, 4, 6])

    winners = ('X-win', 'Draw', 'O-win')

    def __init__(self, squares=[]):
        if len(squares) == 0:
            self.squares = [None for i in range(9)]
        else:
            self.squares = squares

    def available_moves(self):
        return [k for k, v in enumerate(self.squares) if v is None]

    def available_combos(self, player):
        return self.available_moves() + self.get_squares(player)

    def complete(self):
        if None not in [v for v in self.squares]:
            return True
        if self.winner() != None:
            return True
        return False

    def X_won(self):
        return self.winner() == 'X'

    def O_won(self):
        return self.winner() == 'O'

    def tied(self):
        return self.complete() == True and self.winner() is None

    def winner(self, lastcheck = False):
        for player in ('X', 'O'):
            positions = self.get_squares(player)
            for iterator, combo in enumerate(self.winning_combos):
                win = True
                for pos in combo:
                    if pos not in positions:
                        win = False
                if win:
                    self.lastwincombo = combo
                    return player
        return None

    def get_squares(self, player):
        return [k for k, v in enumerate(self.squares) if v == player]

    def make_move(self, position, player):
        self.squares[position] = player

    def alphabeta(self, node, player, alpha, beta):
        if node.complete():
            if node.X_won():
                return -1
            elif node.tied():
                return 0
            elif node.O_won():
                return 1
        for move in node.available_moves():
            node.make_move(move, player)
            val = self.alphabeta(node, get_enemy(player), alpha, beta)
            node.make_move(move, None)
            if player == 'O':
                if val > alpha:
                    alpha = val
                if alpha >= beta:
                    return beta
            else:
                if val < beta:
                    beta = val
                if beta <= alpha:
                    return alpha
        if player == 'O':
            return alpha
        else:
            return beta


def determine(board, player):
    a = -2
    choices = []
    if len(board.available_moves()) == 9:
        return 4
    for move in board.available_moves():
        board.make_move(move, player)
        val = board.alphabeta(board, get_enemy(player), -2, 2)
        board.make_move(move, None)
        board.logic.append("move:{} and it causes:{}; ".format(move,board.winners[val + 1]))
        if val > a:
            a = val
            choices = [move]
        elif val == a:
            choices.append(move)
    return random.choice(choices)


def get_enemy(player):
    if player == 'X':
        return 'O'
    return 'X'
a = sys.argv[1]
a=str.upper(a)
a=list(a)
for i,v in enumerate(a):
    if v == '-':
        a[i] = None
if __name__ == "__main__":
    board = Tic(a)
    dat = {"Board": {"Complete": True, "NextMove": 0, "Winner": None, "WinningCombo": [], "Logic": None, "DBoard": []}}
    #board.show()
    player = 'X'
    if board.complete():
        dat['Board']['Winner'] = board.winner()
        dat['Board']['WinningCombo'] = board.lastwincombo;
        dat['Board']['DBoard'] = board.squares
        print(json.dumps(dat))
        sys.exit()
    player = get_enemy(player)
    computer_move = determine(board, player)
    board.make_move(computer_move, 'O')
    if board.complete():
        dat['Board']['Winner'] = board.winner()
        dat['Board']['WinningCombo'] = board.lastwincombo;
        dat['Board']['DBoard'] = board.squares
        print(json.dumps(dat))
        sys.exit()
    dat['Board']['Logic']    = board.logic
    dat['Board']['DBoard'] = board.squares
    dat['Board']['Complete'] = False
    dat['Board']['NextMove'] = computer_move
    print(json.dumps(dat))
share|improve this question
up vote 12 down vote accepted

As JosephtheDreamer has left the Python to someone else, I'll take up the challenge.

It's a small thing, but the styling for the definition of winning_combos doesn't seem right to me. We can see the start of the definition and what is inside, but the close parenthesis is hidden at the end of one of the lines. I prefer to put it on its own line:

winning_combos = (
    [0, 1, 2], [3, 4, 5], [6, 7, 8],
    [0, 3, 6], [1, 4, 7], [2, 5, 8],
    [0, 4, 8], [2, 4, 6],
)

That way, it's more clear what is inside of the tuple without actually looking through it. I also like to have a trailing comma so that it isn't as easy to add another list and get a SyntaxError for forgetting it.

def __init__(self, square=[]):

It is dangerous to use a mutable object as a default argument. In this case it doesn't matter because you don't use it anyway. As long as you aren't using it, you should use None instead. That way, nothing needs to be created (None has already been defined, but an empty list needs to be created.) When using None, use if square is None: instead of if len(square) == 0:. While we're on that, you could have said if not square: instead of if len(square) == 0:

self.squares = [None for i in range(9)]

Unused variables should be given the name _ to make it clear that they are unused. You don't even need a list comprehension here, though. Just say self.squares = [None] * 9. List comprehensions are useful in similar cases if the object is mutable and you want to instantiate a new one for each element in the list, but None is immutable so that is unnecessary in this case.

if None not in [v for v in self.squares]:

Why create a list comprehension? self.squares is already a list, so you can do if None not in self.squares:

if self.winner() != None:

When comparing to None, you should usually use is None and is not None. If you intend to compare with any Falsey value, you should be using if self.winner(): instead.

That whole function can be combined into one line:

return None not in self.squares or self.winner() is not None:
return self.complete() == True and self.winner() is None

You never need to say == True because if it is equal to True, it already has a boolean value. Just say return self.complete() and self.winner() is None

def winner(self, lastcheck = False):

From PEP8:

Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.

I strongly recommend that you read that whole PEP.

if pos not in positions:
    win = False

To save processing time, you should add break.

return None

That is a completely useless line when given at the end of a function definition. When no return statement is given, the function will return None by default. Even if you were returning at an earlier time than the function would otherwise, you can say return without the None. Edit: As Darkhogg has mentioned in the comments, it isn't entirely useless here. It does improve the clarity of what happens when there is no winner.

if len(board.available_moves()) == 9:
    return 4

What are 9 and 4? This should at least have comments, but even better don't use hard-coded values. Instead, define some constants at the beginning of the file. That makes it easier to change your program later.

def get_enemy(player):
    if player == 'X':
        return 'O'
    return 'X'

This can be simplified to:

def get_enemy(player):
    return ('O', 'X')[player == 'X']

or:

def get_enemy(player):
    return 'O' if player == 'X' else 'X'
a = sys.argv[1]

What if the user doesn't supply any arguments? You'd have a very embarrassing error. You should make sure that sys.argv has the right length before just taking right out of it.

a = str.upper(a)

You don't need to call the class method with an instance as an argument; just use it like an instance method: a = a.upper()

if v == '-':

Another hardcoded value. Who knows? You may want to switch to using * later on. Put a constant at the beginning of the file for your empty-space-filler and use that instead.

if board.complete():
    dat['Board']['Winner'] = board.winner()
    dat['Board']['WinningCombo'] = board.lastwincombo;
    ...

That code is in your program twice. Duplicate code that is more than about three lines (in my opinion) should be in its own function.

share|improve this answer
    
Personally, I consider adding return None in a function that otherwise returns something good for clarity. Not adding it or using just return would be left for functions not intended to return anything. – Darkhogg Mar 20 at 23:33
    
@Darkhogg: You do have a good point there. I think I'll edit. – zondo Mar 21 at 0:06

First, I would suggest simplifying your stack. You're running 3 languages for one simple app. Now given that the main bits of your app are the JS (UI) and Python (AI), you could drop the PHP. You can replace it with Flask, and simply import your AI like any other import.

onclick='ttt.ticclick(this)'

You use jQuery, yet you use inline scripts. I suggest using $(selector).on('event', handler) instead.

ttt = {
  "ticclick": 

Naming could also need work. I don't really understand what ttt is if I didn't know the app was a tic-tac-toe game. I don't even know what ticlick is at all, even if I do know the game was tic-tac-toe.

jQuery.get("code/server.php?mode="+pre+"&data=" + ttt.system.boardencode(ttt.system.collect()), ttt.system.parseServerResponse);

// to

$.get('code/server.php', {
  mode: ...
  data: ...
}).then(function(response){
  // parse response
});

jQuery aliases the jQuery global with $. You can use that instead so it's shorter. Request data can be an object passed as the second argument of $.get. jQuery AJAX operations return a promise interface (fancy name for an object that holds state and has a then method). Use this, as it is more standard.

$('#spinner').css('width', '200px').css('height','200px');

Suggesting you move styling to a CSS class. Just let JS add/remove that class to apply/remove. That way, styling is easier to manage without diving into JS. Same goes for the rest.

console.log('Sending to server: ' + ttt.system.boardencode(ttt.system.collect()))

If the console calls are merely for debugging, I suggest you remove them and use the dev tools breakpoints instead. That way, leaving console calls won't be a habit. It's bad practice especially in real production code. It can break older browsers as well as increase the surface area for bugs. For instance, ttt.system.collect() returning something ttt.system.boardencode() never expected could break your app.

buffer[i] = $(ttt.realz[i]).html();

This is bad practice. Game state should be held in JS. HTML should merely be a representation of that state. When you render, you tell HTML to update based on that state, but that state remains in JS.

I also notice that you have an external stylesheet, yet still have styles in the HTML. I suggest you move that CSS out as well.

I'll leave it at that and let someone else do the Python.

share|improve this answer
1  
One benefit of console.log() over breakpoints is that after something goes wrong, you can look back and see what the program thought it was trying to do. Breakpoints can only show you what the program is trying to do now. – Mark Mar 20 at 3:12
2  
@Mark You do realize that you can set a breakpoint and step through the code. – Joseph the Dreamer Mar 20 at 10:28
2  
@JosephtheDreamer But never step back to see what happened before. – Darkhogg Mar 20 at 23:34
    
@JosephtheDreamer, can you set a breakpoint and step backwards, undoing the program execution? I've never encountered a debugger that could do that. – Mark Mar 21 at 18:17
    
@Mark Then you've never heard of Historical Debugging. Besides, when you breakpoint, you have a stack trace. When you have a stack trace, you know where to put another breakpoint earlier in the stack and trace through forwards. – Joseph the Dreamer Mar 21 at 18:20

PHP

Security

Your code is quite insecure. If I call it as

?mode=foo&data=foo;id

the injected command will be executed. You would need to add escapeshellarg to make it secure.

Misc

  • Bug: You are missing an =.
  • Styling: All upper-case variables names are not all that nice to read.
  • You are never using mode.
share|improve this answer
    
I understand this is a major security flaw. However this was not meant to be for online use, just for localhost and a science fair demonstration. I understand its good practice though. – Joseph A. Mar 20 at 22:43
2  
@bytec0de This is indeed a gaping security hole. The only way to make this code remotely acceptable would be if you wrote a giant # TODO: FIX ARBITRARY COMMAND EXECUTION VULNERABILITY comment in the PHP script. It would really be better not to even write code this way in the first place. (Personally, I wouldn't even trust the PHP developers to have implemented escapeshellarg() correctly for all shells. I'd rather pipe the data to the child process.) – 200_success Mar 21 at 3:52

TBH if you want a responsive interface in a browser (which will require JavaScript) and you have no need to access any real backend stuff like a database during the course of a game, you should just implement the whole thing in HTML and JavaScript. This is total overkill. At first I was thinking port that Python to the PHP layer but there's no need for the PHP layer, either.

share|improve this answer
    
I started out with HTML and then learned LAMP coding and lastly got into the advanced JS coding. I used to also have the same tendency to process everything in the backend but over time I realized that the only time you ever really need to contact the server other than initial page load is when you need to update or retrieve information from a database. – Fedge Mar 20 at 16:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.