CODEWITHSUNDEEP
scalafactory
Amir Afghani
Amir Afghani

Reputation: 38531

Scala factory pattern improve design

I have written the following class and corresponding companion object:

class Tile(val tileCoordinate: Int, val pieceOnTile: Piece) {
    override def toString(): String = {
        if(isOccupied()) {
            pieceOnTile.toString()
        }
        "-"
    }

    def isOccupied(): Boolean = pieceOnTile != null
}

object Tile {
    def apply(coordinate: Int, piece: Piece): Tile = {
        new Tile(coordinate, piece)
    }

    def apply(coordinate: Int): Tile = {
        new Tile(coordinate, ??) // what is the best design here?An Option?
    }
}

My question is, when the factory method to create a Tile without a piece on it is invoked, what is the appropriate argument to pass in to the Tile constructor? I don't want to pass in null, that seems like a poor design choice. Should I have the main constructor take an Option[Piece] and pass in None?

That seems sort of ugly too because when I want to create a tile I'd need to say:

val t = Tile(1, Some(new Knight()));

Upvotes: 0

Views: 1696

Answers (4)

Electric Coffee
Electric Coffee

Reputation: 12104

Personally I would ditch the class and companion object completely, and instead go for a case class:

case class Tile(tileCoordinate: Int, pieceOnTile: Option[Piece] = None) {
  override def toString(): String = pieceOnTile.map(_.toString).getOrElse("-")
}

This allows you to later do the following when checking for values:

... match {
  case Tile(c, Some(p)) => ...
  case Tile(c, None) => ...
}

Having the second parameter default to None allows you to call it as Tile(coord), Tile(coord, None) and Tile(coord, Some(piece))

Upvotes: 4

blintend
blintend

Reputation: 368

Another alternative could be to define empty/occupied cases as separate case classes:

sealed abstract class Tile(val isOccupied: Boolean)

case object EmptyTile extends Tile(false) {
  override def toString: String = "-"
}

case class OccupiedTile(piece: Piece) extends Tile(true) {
  override def toString: String = piece.toString
}

You can use EmptyTile, create OccupiedTile(piece) and you can check a Tile either by the isOccupied flag or by pattern matching.

One possible advantage of this approach is that if you add more methods that assume a contained piece, you will not have to add checks of occupiedness to each method because you could define them only in OccupiedTile in the first place.

Upvotes: 2

scand1sk
scand1sk

Reputation: 1124

I would code the following:

class Tile(val tileCoordinate: Int, val pieceOnTile: Option[Piece]) {

  // A little bit more scala-idiomatic, and a probable bug corrected
  override def toString(): String = pieceOnTile.map(_.toString).getOrElse("-")

  def isOccupied : Boolean = pieceOnTile.isDefined

}

object Tile {

  def apply(coordinate: Int, piece: Piece): Tile = {
    new Tile(coordinate, Some(piece))
  }
  def apply(coordinate: Int): Tile = {
    new Tile(coordinate, None)
  }

}

Upvotes: 3

Michael Zajac
Michael Zajac

Reputation: 55569

If pieceOnTile is optional, then you should use Option for sure. If you don't like the ugliness of having to wrap the Pieces in Some when creating new instances of Tile, then hide it within apply:

class Tile(val tileCoordinate: Int, val pieceOnTile: Option[Piece]) { ... }

object Tile {

  def apply(coordinate: Int, piece: Piece): Tile = new Tile(coordinate, Some(piece))

  def apply(coordinate: Int): Tile = new Tile(coordinate, None)
}

Then even isOccupied will look more scala-like:

 def isOccupied() : Boolean = pieceOnTile.isDefined

Upvotes: 2

Related Questions