10
\$\begingroup\$

I am a long time imperative programmer (mostly C++) taking my first forays into functional programming with Scala.

I am following some online tutorials about functional programming and Scala and I'm trying to piece what I've learned into a representative example (the kind of thing I might do in my day job as a game programmer).

I'm looking for feedback primarily on the functional style and structure but I'm also interested in hearing about Scala or FP naming conventions I may be unaware off.

The program consists of some battler objects that have attack and defend stats. They each get a turn per round to perform an action and the winner is the last battler left.

object Main
{
  case class Battler(name : String, health : Int, damage : Int, mentality : Mentality)
  {
    def decideAction(battlers : Seq[Battler]) : Action = mentality.decideAction(this, battlers)
  }

  abstract class Mentality(healthBeforeHeal : Int, attackStrategy : (Battler, Seq[Battler]) => Battler)
  {
    def decideAction(us : Battler, battlers : Seq[Battler]) : Action =
    {
      (us, battlers) match
      {
        case (Battler(_, ourHealth, _, _), _) if ourHealth < healthBeforeHeal => HealAction(us.name)
        case (_, Nil) => HealAction(us.name)
        case (_, xs) => AttackAction(us.name, attackStrategy(us, xs).name)
      }
    }
  }

  object Mentality
  {
    def getUnhealthiestNotUs(us : Battler, battlers : Seq[Battler]) : Battler =
      battlers.filterNot(_.name == us.name).min(Ordering.by((x : Battler) => x.health))

    def getHealthiestNotUs(us : Battler, battlers : Seq[Battler]) : Battler =
      battlers.filterNot(_.name == us.name).max(Ordering.by((x : Battler) => x.health))

    def getStrongestNotUs(us : Battler, battlers : Seq[Battler]) : Battler =
      battlers.filterNot(_.name == us.name).max(Ordering.by((x : Battler) => x.damage))
  }

  case class AttackingMentality extends Mentality(10, Mentality.getStrongestNotUs)
  case class DefendingMentality extends Mentality(50, Mentality.getUnhealthiestNotUs)

  sealed trait Action
  {
    def apply(battlers : Seq[Battler]) : Seq[Battler]
  }

  object Action
  {
    def perform(battlers : Seq[Battler], actions : Seq[Action]) : Seq[Battler] =
    {
      actions match
      {
        case h :: t => perform(h(battlers), t)
        case Nil => battlers
      }
    }
  }

  case class AttackAction(attackName : String, defendName : String) extends Action
  {
    def apply(battlers : Seq[Battler]) : Seq[Battler] =
    {
      battlers.find(_.name == attackName) match
      {
        case Some(a) => battlers.map(d => if (d.name == defendName) attack(a, d) else d)
        case _ => battlers
      }
    }

    override def toString : String = attackName + " attacks " + defendName

    private def attack(attacker : Battler, defender : Battler) : Battler =
      Battler(defender.name, defender.health - attacker.damage, defender.damage, defender.mentality)
  }

  case class HealAction(name : String) extends Action
  {
    def apply(battlers : Seq[Battler]) : Seq[Battler] = battlers.map(b => if (b.name == name) heal(b) else b)

    override def toString : String = name + " heals themself"

    private def heal(b : Battler) : Battler = Battler(b.name, b.health + 10, b.damage, b.mentality)
  }

  type Round = (Seq[Battler], Int) => (Seq[Action], Int)

  def nextRound : Round =
  {
    (battlers, roundNum) =>
    {
      val actions = for(battler <- battlers) yield(battler.decideAction(battlers))
      (actions, roundNum + 1)
    }
  }

  def fight(battlers: Seq[Battler]) : Unit =
  {
    println("Begin Fight")
    println("Fighters: " + battlers.map(x => x.name + " Health: " + x.health.toString).mkString(", "))

    def loop(battlers: Seq[Battler], roundNum : Int) : Seq[Battler] =
    {
      battlers match
      {
        case Nil => battlers
        case h :: Nil => battlers
        case _ =>
        {
          val (actions, newRoundNum) = nextRound(battlers, roundNum)

          println("Begin Round: " + newRoundNum)
          println(actions.mkString("\n"))
          println("End Round")

          val stillAlive = Action.perform(battlers, actions).filter(_.health > 0)
          println("Still Alive: " + stillAlive.map(x => x.name + " Health: " + x.health.toString).mkString(", "))

          loop(stillAlive, newRoundNum)
        }
      }
    }

    val winners = loop(battlers, 0)

    println("End Fight")
    println("Winner: " + winners.map(x => x.name + " Health: " + x.health.toString).mkString(", "))
  }

  def main(args : Array[String]) : Unit =
  {
    val battlers = List(
      Battler(name = "Wizard", health = 60, damage = 60, mentality = DefendingMentality()),
      Battler(name = "Bot", health = 90, damage = 20, mentality = AttackingMentality()),
      Battler(name = "Boxer", health = 120, damage = 9, mentality = AttackingMentality())
    )

    fight(battlers)
  }
}
\$\endgroup\$
  • \$\begingroup\$ One aspect I'm not particularly pleased with is applying the actions as despite having direct access to the battler objects I need to search the list in order to produce the new state. \$\endgroup\$ – Downie Nov 9 '14 at 16:33
  • 1
    \$\begingroup\$ Replace Nil with Seq() and :: with +:. For example WrappedArray is Seq but is not List. See val seq:Seq[Int] = Array(1,3);seq match{ case h::t => h} and val seq:Seq[Int] = Array(1,3);seq match{ case h+:t => h} :: is operator on List - you will receive MatchError for Seq which are not a List. \$\endgroup\$ – Andrzej Jozwik Nov 9 '14 at 20:47
6
\$\begingroup\$

I have the feeling that this code is extremely OOP – and that is fine, since Scala is a multi-paradigm language. However, I disagree with the factoring of your code.

Here are all the classes, objects, and methods in your code:

object Main
  class Battler
    val name: String
    val health: Int
    val damage: Int
    val mentality: Mentality
  abstract class Mentality
    def decideAction(Battler, Seq[Action]): Action
  object Mentality
    def getUnhealthiestNotUs(Battler, Seq[Battler]): Battler
    def getHealthiestNotUs(Battler, Seq[Battler]); Battler
    def getStrongestNotUs(Battler, Seq[Battler]): Battler
  class AttackingMentality extends Mentality(10, Mentality.getStrongestNotUs)
  class DefendingMentality extends Mentality(10, Mentality.getUnhealthiestNotUs)
  trait Action
    def apply(Seq[Battler]): Seq[Battler]
  object Action
    def perform(Seq[Battler], Seq[Action]): Seq[Battler]
  class AttackAction extends Action
    val attackName: String
    val defendName: String
    def apply(Seq[Battler]): Seq[Battler]
    def toString(): String
    def attack(Battler, Battler): Battler
  class HealAction extends Action
    val name: String
    def apply(Seq[Battler]): Seq[Battler]
    def toString(): String
    def heal(Battler): Battler
  type Round = (Seq[Battler], Int) => (Seq[Action], Int)
  def nextRound(): Round
  def fight(Seq[Battler]): Unit
  def main(Array[String]): Unit

The first problem is that everything is inside the Main object. I'd encourage you to reduce nesting, and more clearly separate the game mechanics from the wrapper that offers an user interface. This also implies that AttackAction.toString and HealAction.toString should not be used to generate user-visible output.

In my refactoring of the code, I have introduced two classes Game and GameState. A Game sets up the initial GameState and can then be used as a collection of subsequent game states. My Main object includes a displayGame method that contains the whole user interface.

def displayFight(game: Game): Unit = {
  println("Begin Fight")
  println("Fighters: %s".format(showBattlers(game.battlers)))
  for (state <- game) {
    println("Round %d".format(state.round))
    for (action <- state.actions) {
      println(showAction(action))
    }
    println("Still Alive: %s".format(showBattlers(state.battlers)))
  }
  println("End Fight")
  println("Winner: %s".format(game.last.battlers.head.name))

  def showBattlers(fighters: Seq[Battler]): String =
    fighters map (f => "%s (health %d)".format(f.name, f.health)) mkString ", "

  def showAction(action: Action): String = action match {
    case Action.Attack(attacker, defender) => "%s attacks %s".format(attacker.name, defender.name)
    case Action.Heal(battler) => "%s heals themself".format(battler.name)
  }
}

If you read through that code, you will notice I use Action.Attack rather than your AttackAction. I moved the Attack and Heal classes into the Action companion object to achieve a kind of namespacing. Also, these actions now take Battler instances as parameters rather than just Strings. While that isn't necessary, I find it to be a bit more elegant.

Your Mentality hierarchy is severly screwed. You have an abstract class Mentality and extend it to case class AttackingMentality extends Mentality(10, Mentality.getStrongestNotUs). Why is this a problem?

  1. You should not declare a case class without an explicit list of fields: case class Foo() is OK, but case class Foo isn't. Why? The point of case classes is to make pattern matching easier. The language automatically creates a kind of reverse constructor that fits the pattern matching syntax. A case class without constructor arguments is likely to be a mistake – instances of case classes are compared using structural equivalence, not by object identity.

  2. The case classes add nothing. No fields, no behaviour. They merely add more types to your program – and nowhere do you inspect the type of Mentality instances. The constructor of these classes is absolutely useless, so let's remove these classes, and just use objects for each kind of mentality instead.

  3. As the empty AttackingMentalityy and DefendingMentality classes show, Mentality shouldn't be abstract in the first place.

You might have found this weird architecture because you were thinking of the Strategy Pattern. An abstract class declares but not defines a virtual method that describes some part of the algorithm. Subclasses must override this method. In Scala, there are two other variants of this pattern:

  • Instead of abstract base classes, traits can often be used for the same job. This requires a class hierarchy.
  • Instead of providing the virtual method through subclassing, it can be passed in as a first-class function through the constructor. This makes the class hierarchy usually implied by the strategy pattern unnecessary.

Anyway, I would write this as

class Mentality(...) {
  ...
}

object Mentality {
  val Attacking = new Mentality(10, getStrongestNotUs)
  val Defensive = new Mentality(50, getUnhealthiestNotUs)
  ...
}

// Battler(name = "Wizard", health = 60, damage = 60, mentality = Mentality.Defensive)

Now on to Seq. The Seq trait is for sequences, that is some ordered list. However, your code doesn't depend anywhere on the order of battlers (except when printing out the list of fighters). We could therefore pick a more general trait such as Traversable, or a trait with different guarantees such as Set. We ought to pick something that allows us to write

def apply(battlers : Seq[Battler]) : Seq[Battler] = battlers.map(b => if (b.name == name) heal(b) else b)

more efficiently. Currently, applying all actions has \$O(n^2)\$ complexity, which scales really bad. We could use a (possibly mutable) Map[String, Battler]. Then, complexity would be lowered to average \$O(n \log n)\$ or \$O(n)\$, depending on what kind of map is used. The snippet below assumes immutable maps; this should still be at least as efficient as iterating through the whole list each time.

case class Heal(battler: Battler) extends Action {
  def apply(battlers: Map[String, Battler]): Map[String, Battler] =
    updated(battlers, battler.name) { battler =>
      battler.withHealth(battler.health + 10)
    } getOrElse {
      throw new IllegalStateException("battlers must contain battler")
    }
}

Further points that could be improved include:

  • Bracing style. The opening brace should not be on a line of its own. This is not a purely stylistic issue, as Scala's automatic semicolon insertion might otherwise end a statement at unintended locations
  • Traversable has a maxBy operation that allows you to easily specify a sort key. No need to manually create an Ordering.
  • You are using far more pattern matching than necessary. Many of your uses could be replaced by existing abstractions such as Option.getOrElseor Traversable.foldLeft.

The code I ended up with has these classes and methods:

object Main extends App
  def displayFight(Game)
class Game extends Traversable[GameState]
  val first: GameState
  val last: GameState
  def foreach[U](f: GameState => U): Unit
class GameState
  val round: Int
  val battlers: Traversable[Battler]
  val actions: Traversable[ACtions]
  val hasNext: Boolean
  val next: GameState
  def getLast(): GameState
class Battler
  val name: String
  val health: Int
  val damage: Int
  def decideAction(Traversable[Battler]): Action
  def withHealth(Int): Battler
class Mentality
  def decideAction(Battler, Traversable[Battler]): Action
object Mentality
  val Attacking: Mentality
  val Defensive: Mentality
trait Action
  def apply(Map[String, Battler]): Map[String, Battler]
object Action
  class Attack
    val attacker: Battler
    val defender: Battler
    def apply(Map[String, Battler]): Map[String, Battler]
  class Heal
    val battler: Battler
    def apply(Map[String, Battler]): Map[String, Battler]

And this is the code:

import scala.annotation.tailrec

object Main extends App {
  displayFight(
    new Game(
      new Battler(name = "Wizard", health = 60, damage = 60, mentality = Mentality.Defensive),
      new Battler(name = "Bot", health = 90, damage = 20, mentality = Mentality.Attacking),
      new Battler(name = "Boxer", health = 120, damage = 9, mentality = Mentality.Attacking)
    )
  )

  def displayFight(game: Game): Unit = {
    println("Begin Fight")
    println("Fighters: %s".format(showBattlers(game.battlers)))
    for (state <- game) {
      println("Round %d".format(state.round))
      for (action <- state.actions) {
        println(showAction(action))
      }
      println("Still Alive: %s".format(showBattlers(state.battlers)))
    }
    println("End Fight")
    println("Winner: %s".format(game.last.battlers.head.name))

    def showBattlers(fighters: Traversable[Battler]): String =
      fighters map (f => "%s (health %d)".format(f.name, f.health)) mkString ", "

    def showAction(action: Action): String = action match {
      case Action.Attack(attacker, defender) => "%s attacks %s".format(attacker.name, defender.name)
      case Action.Heal(battler) => "%s heals themself".format(battler.name)
    }
  }
}

class Game(val battlers: Battler*) extends Traversable[GameState] {
  lazy val first: GameState = new GameState(0, battlers, Nil)

  override lazy val last: GameState = first.getLast

  def foreach[U](f: GameState => U): Unit = {
    @tailrec
    def loop(state: GameState): Unit = {
      f(state)
      if (state.hasNext)
        loop(state.next)
    }
    loop(first)
  }
}

class GameState(val round: Int, val battlers: Traversable[Battler], val actions: Traversable[Action]) {
  val hasNext: Boolean = battlers.size > 1

  def getLast(): GameState = {
    @tailrec
    def loop(state: GameState): GameState =
      if (state.hasNext)
        loop(state.next)
      else
        state
    loop(this)
  }

  lazy val next: GameState = {
    if (!hasNext) {
      throw new IllegalStateException("cannot inspect next element when hasNext is false")
    }
    val actions = battlers map (_.decideAction(battlers))
    val stillAlive = {
      val battlersAsMap = battlers.map(b => (b.name, b)).toMap
      val processedBattlers: Map[String, Battler] =
        (actions foldLeft battlersAsMap)((battlers, action) => action(battlers))
      processedBattlers.values.filter(_.health > 0)
    }
    new GameState(round + 1, stillAlive, actions)
  }
}

class Battler(val name: String, val health: Int, val damage: Int, mentality: Mentality) {
  def decideAction(battlers: Traversable[Battler]): Action =
    mentality.decideAction(this, battlers)

  def withHealth(newHealth: Int) = new Battler(name, newHealth, damage, mentality)
}

class Mentality(healthBeforeHeal: Int, attackStrategy: (Battler, Traversable[Battler]) => Battler) {
  def decideAction(us: Battler, battlers: Traversable[Battler]): Action =
    if (us.health < healthBeforeHeal)
      Action.Heal(us)
    else if (battlers.isEmpty)
      Action.Heal(us)
    else
      Action.Attack(us, attackStrategy(us, battlers))
}

object Mentality {
  val Attacking = new Mentality(10, getStrongestNotUs)
  val Defensive = new Mentality(50, getUnhealthiestNotUs)

  private def getUnhealthiestNotUs(us: Battler, battlers: Traversable[Battler]): Battler =
    battlers.filterNot(_.name == us.name).minBy(_.health)

  private def getHealthiestNotUs(us: Battler, battlers: Traversable[Battler]): Battler =
    battlers.filterNot(_.name == us.name).maxBy(_.health)

  private def getStrongestNotUs(us: Battler, battlers: Traversable[Battler]): Battler =
    battlers.filterNot(_.name == us.name).maxBy(_.health)
}

sealed trait Action {
  def apply(battlers: Map[String, Battler]): Map[String, Battler]
}

object Action {
  private def updated[K, V](map: Map[K, V], key: K)(action: V => V): Option[Map[K, V]] =
    if (map contains key)
      Some(map.updated(key, action(map(key))))
    else
      None

  case class Attack(attacker: Battler, defender: Battler) extends Action {
    def apply(battlers: Map[String, Battler]): Map[String, Battler] =
      updated(battlers, defender.name) { defender =>
        defender.withHealth(defender.health - attacker.damage)
      } getOrElse {
        throw new IllegalStateException("battlers must contain defender")
      }
  }

  case class Heal(battler: Battler) extends Action {
    def apply(battlers: Map[String, Battler]): Map[String, Battler] =
      updated(battlers, battler.name) { battler =>
        battler.withHealth(battler.health + 10)
      } getOrElse {
        throw new IllegalStateException("battlers must contain battler")
      }
  }
}
\$\endgroup\$
  • \$\begingroup\$ I'd never normally put everything in one file or in main, that was simply to make it easier for others to run. I'm glad you recommend not using toString as I've never liked that about Java and only used it as the tutorials I was following used it in the manner. You are correct about my choice of data structure effectively it's a database so a map makes more sense. \$\endgroup\$ – Downie Nov 10 '14 at 8:13
  • \$\begingroup\$ Is it more common when using Scala to use pattern matching to determine which function to call rather than using class polymorphism i.e. decideAction etc? This would mean that adding a new action requires touching multiple parts of the code base and not just one class. Your correct that the mentality subclasses are just values \$\endgroup\$ – Downie Nov 10 '14 at 8:16
  • \$\begingroup\$ That's unfortunate about the Egyptian braces I've always found them hard to read 😉 \$\endgroup\$ – Downie Nov 10 '14 at 8:18
  • \$\begingroup\$ Also my 2nd comment should read showAction and not decideAction \$\endgroup\$ – Downie Nov 10 '14 at 8:22
  • 1
    \$\begingroup\$ @Downie: Each class should have a single responsibility only, and many of the classes here are merely introspectable closures. In OOP, the visitor pattern is often used to effectively define extension methods. Pattern matching is a viable alternative when no new subtypes will be added later, as it's typically far less code. Here, trait Action is sealed, so all subtypes must be declared in the same compilation unit. Yes, adding an action involves rewriting much code (the compiler will help find unmatched cases), but changing the UI now only touches one place in the code. \$\endgroup\$ – amon Nov 10 '14 at 10:20
4
\$\begingroup\$

I wrote a different version which I tried to make more functional than OO. It is fully immutable.

Note that the attributes of Battler are all constant except for the health, so I took that out of Battler and used a Map[Battler, Health]. This map is basically the state of the system. It is important to clearly delineate the mutable state from the start, for any problem.

You combine the healing strategy and attacking strategy in a very specific way in Mentality. Instead I made the healing and attacking strategies subtypes of a more general Strategy type. This allows for more flexibility in the future.

I honestly wrote this more as an exercise for myself, but hopefully it will be helpful to you too.

object BattleStackOverflow extends App {

  type Damage = Int
  type Health = Int
  type HealthChange = Health
  type Action = Seq[(Battler, HealthChange)]
  case class Battler(name: String, damage: Damage, strategy: Strategy)
  type BattlersHealth = Map[Battler, Health]
  type Strategy = (Battler, BattlersHealth) => Action

  object ActionUtil {
    val doNothingAction: Action = Nil
    def selfHeal(battler: Battler): Action = List((battler, 10))
    def buildAttackAction(battler: Battler, attackedEnemy: Battler): Action = 
                                               List((attackedEnemy, -battler.damage))
  }
  import ActionUtil._

  object StrategyUtil {
    val doNothingStrategy: Strategy = (_, _) => doNothingAction

    type ConditionalStrategy = (Battler, BattlersHealth) => Option[Action]
    def chainStrategies(conditionalStrategies: Seq[ConditionalStrategy], fallbackStrategy: Strategy): Strategy = {
      (battler: Battler, battlersHealth: BattlersHealth) =>
        conditionalStrategies
          .map(_(battler, battlersHealth))
          .find(_.isDefined)
          .map(_.get)
          .getOrElse(fallbackStrategy(battler, battlersHealth))
    }

    case class HealingStrategy(healthThreshold: Health) extends ConditionalStrategy {
      override def apply(battler: Battler, battlersHealth: BattlersHealth): Option[Action] = {
        if (battlersHealth(battler) < healthThreshold) Some(selfHeal(battler)) else None
      }
    }

    type PickEnemySubStrategy = BattlersHealth => Battler
    val pickUnhealthiestStrategy: PickEnemySubStrategy = enemies => enemies.minBy(_._2)._1
    val pickHealthiestStrategy: PickEnemySubStrategy = enemies => enemies.maxBy(_._2)._1
    val pickStrongestStrategy: PickEnemySubStrategy = enemies => enemies.keys.maxBy(_.damage)

    def buildAttackStrategy(attackSubStrategy: PickEnemySubStrategy): Strategy = (battler, battlersHealth) => {
      val chosenEnemy = attackSubStrategy(battlersHealth - battler)
      buildAttackAction(battler, chosenEnemy)
    }
  }
  import StrategyUtil._

  object Game {
    def completeOneRound(battlersHealth: BattlersHealth): BattlersHealth = {
      val actions = battlersHealth.keys.toList.map(battler => battler.strategy(battler, battlersHealth)).flatten
      val groupedTotalHealths = (actions ++ battlersHealth.toList).groupBy(_._1).mapValues(_.map(_._2))
      val simplifiedTotalHealths = groupedTotalHealths.mapValues(healthChanges => healthChanges.reduce(_ + _))
      val survivors = simplifiedTotalHealths.filter(_._2 > 0)
      survivors
    }

    def playGame(initialState: BattlersHealth): Stream[BattlersHealth] = {
      def playInfiniteGame(initialState: BattlersHealth): Stream[BattlersHealth] = {
        def loop(battlersHealth: BattlersHealth): Stream[BattlersHealth] =  
                     battlersHealth #:: loop(completeOneRound(battlersHealth))
        loop(initialState)
      }
      val (validMoves, stopMoves) = playInfiniteGame(initialState).span(_.size >= 2)
      validMoves ++ stopMoves.take(1)
    }
  }
  import Game._

  val defensiveStrategy1: Strategy = chainStrategies(List(HealingStrategy(50)),
            buildAttackStrategy(pickStrongestStrategy))
  val offensiveStrategy1: Strategy = chainStrategies(List(HealingStrategy(10)),
            buildAttackStrategy(pickUnhealthiestStrategy))

  val initialState = Map(
    Battler("Wizard", 10, defensiveStrategy1) -> 60,
    Battler("Bot", 4, offensiveStrategy1) -> 90,
    Battler("Boxer", 2, offensiveStrategy1) -> 120)

  playGame(initialState).foreach(println(_))

}
\$\endgroup\$
  • \$\begingroup\$ Really good point about health being the only stateful part of the application and therefore it being logical to separate that from the battler \$\endgroup\$ – Downie Nov 11 '14 at 9:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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