Return to Revisions

#General Feedback ##Large Methods Some of the class methods are really large - e.g. ChessGame ::get_legal_moves_list() which consumes ~150 lines. There is a lot of redundancy - especially in the code to assign $directions_list. That code should be moved out to a separate method. Generally whenever a method goes beyone ~10-15 lines you should consider refactoring it. In addition to readability it will also help with testing.

In that method, there are 4 calls to ::add_slide_and_slidecapture_moves_to_moves_list() and those could all likely be simplified to a single call whenever the piece type is bishop, rook, queen or king.

//the array below could be stored in a constant - e.g. CAPTURABLE_PIECE_TYPES
else if ( in_array($piece->type, array('bishop', 'rook', 'queen', 'king')) {
    //getDirectionsListForPiece is a method that could accept the piece object, or it may be simpler to pass $piece->type
    $directions_list = $this->getDirectionsListForPiece($piece); 
    $moves = $this->add_slide_and_slidecapture_moves_to_moves_list($directions_list, 7, $moves, $piece, $color_to_move, $board);
}

Because $directions_list doesn't appear to be used later in that method, perhaps it could be removed from the list of arguments for all methods that accept it, and those methods could fetch it when necessary.

For more on cleaning up code, check out this video of a presentation Rafael Dohms talk about cleaning up code (or see the slides here).

##Exception Handling Exceptions are thrown but never caught. Currently if an exception is thrown, that is displayed to the user, like the one I see when I change the URL of a move:

Fatal error: Uncaught exception 'Exception' with message 'ChessBoard Class - Invalid FEN' in /home/clania/clania.net/admiraladama/chess_v2/ChessBoard.php:54 Stack trace: #0 /home/clania/clania.net/admiraladama/chess_v2/index.php(23): ChessBoard->set_fen('rnbqkbnr/pppp1p...') #1 {main} thrown in /home/clania/clania.net/admiraladama/chess_v2/ChessBoard.php on line 54

Ideally those would be handled - perhaps in Index.php with a try/catch that may set the value of an error variable and the view would display that error message instead of the board.

###PHP mixed within HTML Generally it is best to separate the business logic (PHP) from the view (HTML). Using a template engine could help for this aspect - e.g. Smarty, Twig, etc. For more information on this topic, prefer to this article.

#More Specific points ##PHP ###Declaring Instance/Member variables The class definitions appear to use the PHP 4 style declarations for instance variables (i.e. var). Note that while support of this syntax isn't currently deprecated, it may be in the future:

Note: The PHP 4 method of declaring a variable with the var keyword is still supported for compatibility reasons (as a synonym for the public keyword). In PHP 5 before 5.1.3, its usage would generate an E_STRICT warning.¹

###Getting the timestamp with microseconds Index.php starts with the following lines:

$time = microtime();

$time = explode(' ', $time);
$time = $time[1] + $time[0];
$start = $time;

Some would argue that readability suffers here because on the first line, $time is a string, then on the second line it is an array and then on the third line it is a string again.

This appears to function identically to the Example 1: Example #1 Timing script execution with microtime() on the PHP Documentation for microtime(). Notice that the function in that example contains the comment:

/**

 * Simple function to replicate PHP 5 behaviour
 */
 

And notice the next example is titled Example #2 Timing script execution in PHP 5, where it simply calls microtime() with true for the parameter get_as_float. So instead of using those first three lines of Index.php could simply be replaced with the call to microtime(true) for the same functionality.

$start = microtime(true);

The same applies to the identical code within View.php.

###Error Reporting Index.php also contains the following line:

error_reporting(-1);

According to the code in Example #1 error_reporting() examples in the documentation for error_reporting(), E_ALL can also be used for that (presuming the PHP version is 5.4 or greater - otherwise E_STRICT won't be included in that):

// Report all PHP errors (see changelog)

error_reporting(E_ALL);

// Report all PHP errors

error_reporting(-1);    

Using the constant like that (i.e. error_reporting(E_ALL);) is more readable for anyone who doesn't remember what -1 signifies when passed to that

##JS ###DOM ready function the documentation for .ready() states:

jQuery offers several ways to attach a function that will run when the DOM is ready. All of the following syntaxes are equivalent:

• $( handler )

• $( document ).ready( handler )
• $( "document" ).ready( handler )
• $( "img" ).ready( handler )
• $().ready( handler )

As of jQuery 3.0, only the first syntax is recommended; the other syntaxes still work but are deprecated.²

##CSS ###Simplification of padding for status box The four padding styles can be simplified from:

padding-top: 2px;

padding-bottom: 2px;
padding-left: 4px;
padding-right: 4px;

To the following:

padding: 2px 4px;

Because:

When two values are specified, the first padding applies to the top and bottom, the second to the left and right. ³

¹_{http://php.net/manual/en/language.oop5.visibility.php#language.oop5.basic.class.class.name} ²_{http://api.jquery.com/ready/ ³https://developer.mozilla.org/en-US/docs/Web/CSS/padding#Syntax}