HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

"Pure" functional simple Scala battle game simulation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codereviewstackoverflowfunctional-programmingscalagamebattle-simulation
battlesimplepuresimulationscalagamefunctional

Problem

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 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.g

Solution

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  "%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?

-
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.

-
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.

-
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 anywh

Code Snippets

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
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)
  }
}
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)
def apply(battlers : Seq[Battler]) : Seq[Battler] = battlers.map(b => if (b.name == name) heal(b) else b)
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")
    }
}

Context

StackExchange Code Review Q#69297, answer score: 6

Revisions (0)

No revisions yet.