Är det fult så är det fel!

Vill bara börja med att redogöra lite för min viktigaste och trognaste designprincip som jag lutat mig mot ett antal år. Den lyder: Är det fult så är det fel! Den är mycket bra att använda sig av när man funderar över huruvida man behöver skriva om sin (eller annans) kod eller inte. Om svaret på frågan är det fult är ja mår åtminstone delar av koden bra av att skrivas om.

Ett designbeslut kan kännas okej när man gör det, men hundra rader kod senare upptäcker man att det plötsligt ser fult ut. I mitt förra gjorde jag ett designbeslut att låta klassen Position hantera placering av Piece men inte hantera rotationsläget av den samma (det var gjort så i Java-versionen). När jag lade till hantering av den nya klassen Move till Position blev problemen tydliga. Move hanterar både x,y och rotation och det finns stöd för att sätta en Move direkt i Position via metoden movePiece(move: Move). Att det samtidigt gick att rotera en Piece via en instans utanför Positions kontroll ledde till extrakod som dessutom inte var helt uppenbar varför den behövdes när man tittade på den.

Det blev helt enkelt fult och är det fult så är det fel. Jag hade i och för sig haft en känsla hela tiden att det kanske inte var så bra att låta Piece hantera sin egen rotation, men valde ändå att testa detta då det verkade bli en enklare hantering (vilket det också var till en början). Fick det även påpekat av en kollega som ställde sig undrande till min design! Problemet löste sig genom att låta Position ha hand om både bitens placering och rotationsläge. Ett annat resultat av manövern blev att koden från PieceType fick flytta in till Piece och att PieceType kunde tas bort. Position ser nu ut så här:

package net.sf.tetrisai

import move.Move
import piece.Piece
import settings.GameSettings

class Position(board: Board, val piece: Piece, settings: GameSettings) {
  val boardWidth = board.width
  val boardHeight = board.height
  var pieceMove = settings.pieceStartMove(board.width, piece)
  val rotationDirection = settings.rotationDirection

  def setPiece() = piece.shape(pieceMove.rotation).dots.foreach(dot => board.set(dot + pieceMove, piece.number))
  def clearPiece() = piece.shape(pieceMove.rotation).dots.foreach(dot => board.clear(dot + pieceMove))
  def rotatePiece() = pieceMove = pieceMove.rotate(rotationDirection, piece.rotationModulus)
  def movePiece(move: Move) = pieceMove = move
  def isValidMove: Boolean = isValidMove(pieceMove)
  def isValidMove(move: Move): Boolean = {
    try {
      (piece.shape(move.rotation).dots.foldLeft(0) { (sum,dot) => sum + board.get(dot + move) }) == 0
    } catch {
      case e: IndexOutOfBoundsException => false
    }
  }
}

I metoden isValidMove på rad 18 låter jag språket självt hålla reda på om biten hamnat utanför brädets kanter genom att låta den fånga IndexOutOfBoundsException. Detta är både en presntandaoptimering och tar samtidigt bort en hel massa boundary-checks som jag annars skulle behövt lägga till.

Klassen Move ser ut så här:

package net.sf.tetrisai.move

import net.sf.tetrisai.piece.Point

object Move {
  def apply(y: Int, x: Int, rotation: Int) = new Move(y, x, rotation)
}

class Move(y: Int, x: Int, val rotation: Int) extends Point(x,y) with Ordered[Move] {
  def down = new Move(y + 1, x, rotation)
  def left = new Move(y, x - 1, rotation)
  def right = new Move(y, x + 1, rotation)
  def rotate(rotationType: RotationDirection, rotationModulus: Int) = new Move(y, x, (rotation + rotationType.direction) & rotationModulus)

  def compare(that: Move): Int = {
    if (y != that.y)
      y compare that.y
    else
      if (x != that.x)
        x compare that.x
      else
        rotation compare that.rotation
  }

  override def equals(that: Any) = that match {
    case other: Move => y == other.y && x == other.x && rotation == other.rotation
    case _ => false
  }

  override def toString = "Move(" + y + "," + x + "," + rotation + ")"
}

Det som kan vara värt att nämna här är att jag ärver från klassen Point och att jag gjort klassen sorterbar (mitt test vill ha en förutsägbar ordning på listor av Move). Det är en väldigt elegant syntax när man ska delegera vidare konstruktorns argument till den ärvda klassen (rad 9): extends Point(x,y). En ny instansvariabel position (rad 9) har också tillkommit vilket markeras med nyckelordet val (immutable value).

En annan detalj som kan se konstigt ut för en Javaprogrammerare är syntaxen på rad 17 y compare that.y. Detta tolkas av Scala som y.compare(that.y) och det är en smaksak vilket man väljer!

Vän av ordning undrar nu säkert varför Move ärver från Point. Anledningen är att Move ibland ska hanteras som en Point vilket görs av metoderna setPiece, clearPiece och isValidMove i klassen Position där vi adderar en Point med en Move (rad 13, 14 och 20).

För att stödja olika rotationsriktningar (med- och moturs) i metoden rotate (rad 13 i klassen Move) har jag lagt till den abstrakta klassen RotationDirection:

package net.sf.tetrisai.move

abstract class RotationDirection {
  val direction: Int
}

Själva klassen är markerad som abstrakt och attributet direction är här inte satt vilket implicit gör den abstrakt. Mina två implementationer av denna klass måste därför implementera detta värde. ClockwiseRotation:

package net.sf.tetrisai.move

class ClockwiseRotation extends RotationDirection {
  val direction = 1
}

AnticlockwiseRotation:

package net.sf.tetrisai.move

class AnticlockwiseRotation extends RotationDirection {
  val direction = -1
}

Målet med detta projekt är att datorn själv ska kunna spela Tetris. Ett steg på vägen är att leta fram giltiga flytt för en viss position. Klassen ValidMoveCalculator hanterar detta, tillhörande test ser ut så här:

package net.sf.tetrisai.move

import org.junit.Test
import net.sf.tetrisai.settings.DefaultGameSettings
import net.sf.tetrisai.{Position, Board, BaseTest}
import net.sf.tetrisai.piece.PieceS

class ValidMoveCalculatorTest extends BaseTest {

  @Test def validMoves() {
    val board = Board(Array(
      "#-----#",
      "#-----#",
      "#--X--#",
      "#--X--#",
      "#######"))
    val position = new Position(board, new PieceS, new DefaultGameSettings)
    val validMoveCalculator = new ValidMoveCalculator(position)
    val moves = validMoveCalculator.validMoves.sorted[Move]

    moves should be (List(Move(0,1,0), Move(0,2,0), Move(0,2,1), Move(1,0,0), Move(1,0,1), Move(1,3,1)))
  }
}

Implementationen av ValidMoveCalculator:

package net.sf.tetrisai.move

import net.sf.tetrisai.Position

class ValidMoveCalculator(val position: Position) {
  var moves: List[Move] = Nil
  val visitedMoves = Array.fill(position.boardHeight, position.boardWidth) { 0 }

  private def markAsVisited(move: Move) { visitedMoves(move.y)(move.x) |= 1 << move.rotation }
  private def isUnvisitedMove(move: Move): Boolean = {
    try {
      (visitedMoves(move.y)(move.x) & (1 << move.rotation)) == 0
    } catch {
      case e: IndexOutOfBoundsException => false
    }
  }

  def validMoves = {
    moves = List.empty[Move]
    calculateValidMoves
    moves
  }

  private def calculateValidMoves {
    while (isUnvisitedMove(position.pieceMove) && position.isValidMove) {
      val move = position.pieceMove
      markAsVisited(move)
      if (position.isValidMove(move down)) {
        position.movePiece(move down)
        calculateValidMoves
      } else {
        moves = move :: moves
      }
      position.movePiece(move left)
      calculateValidMoves
      position.movePiece(move right)
      calculateValidMoves

      position.movePiece(move)
      position.rotatePiece
    }
  }
}

Hanteringen att ta fram giltiga flytt var i C++ versionen rätt hårt optimerad och
svårbegriplig. Här har jag i stället valt att göra den rekursiv. Då vi med flaggor i visitedMoves markerar vilka sökvägar som redan behandlats bör vi även får bra prestanda. Anledninge till att jag tjatar om bra prestanda är att finjusteringen av algoritmerna som väljer bästa plats för en bit kräver bra prestanda (återstår att implementera). Den låter datorn spela x antal miljoner bitar per parametersättning. Sedan görs en statistiks jämförelse mellan de olika parametersättningarna för att hitta bästa värde för respektive parameter.