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?
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 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")
}
}
}
::with+:. For example WrappedArray is Seq but is not List. Seeval seq:Seq[Int] = Array(1,3);seq match{ case h::t => h}andval 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