Fight simulator

Question

I have been learning Scala for a couple months and want to get a more concrete understanding of proper oop practice. I find it difficult to optimally use traits, abstract classes and inheritance.

import scala.util.Random

class Position(posX: Int, posY: Int){
    val x = (posX + 16) % 16
    val y = (posY + 16) % 16
    def distance(that: Position) = math.sqrt((this.x - that.x)*(this.x - that.x) + (this.y - that.y)*(this.y - that.y))
    override def toString() = s"($x, $y)"
}

abstract class Movable(var position: Position){
    def right():Unit = position = new Position(position.x + 1, position.y)
    def left(): Unit = position = new Position(position.x - 1, position.y)
    def up():   Unit = position = new Position(position.x, position.y + 1)
    def down(): Unit = position = new Position(position.x, position.y - 1)
    def distance(that: Movable) = this.position.distance(that.position)
}

abstract class Fighter(p: Position, h: Int, d: Int, r: Int) extends Movable(p){
    var health: Int = h
    val damage: Int = d
    val range: Int = r
    def strike(enemy: Fighter): Boolean = {
        if (inRange(enemy)) {
            hit(enemy)
            return true
        }
        else return false
    }
    def hit(enemy: Fighter): Unit = enemy.getHit(damage)
    def getHit(dmg: Int): Unit = health = health - dmg
    def inRange(enemy: Fighter): Boolean = distance(enemy) <= range
    def alive(): Boolean = health > 0
}

class Archer(p: Position, h: Int = 50, d: Int = 4, r: Int = 4) extends Fighter(p, h, d, r){
    override def toString() = s"Archer at $position with $health health"
}

class Warrior(p: Position, h: Int = 75, d: Int = 7, r: Int = 2) extends Fighter(p, h, d, r){
    override def toString() = s"Warrior at $position with $health health"
}

def turn(f: Fighter, enemy: Fighter) = {

    println(s"$f")
    Random.nextInt(15) % 4 match {
        case 0 => f.right
        case 1 => f.left
        case 2 => f.up
        case 3 => f.down
    }
    if(f.strike(enemy)){
        println(s"$f hit $enemy for ${f.damage} damage")
    }
}

def simFight() = {
    val a = new Archer(new Position(Random.nextInt(15), Random.nextInt(15)))
    val w = new Warrior(new Position(Random.nextInt(15), Random.nextInt(15)))

    var i = 0
    while(a.alive && w.alive){
        i % 2 match {
            case 0 => turn(a, w)
            case 1 => turn(w, a)
        }
        i = i + 1
    }
    if(a.alive) println("Archer wins!") else println("Warrior wins!")
}

Answer 1

Position

You have overloaded Position, which really should be the simplest of types, with the properties of the map/grid on which they are placed. I really don't think it should contain anything more than the co-ordinates and toString. Your type has too much in it (while your game doesn't have enough organised information about your map)

Map Size

You have made the size of your map a fundamental property of positions, hard-coded it in and not explicitly named it anywhere (as a property or constant). Firstly, magic numbers are usually a bad thing, secondly, the size of the map really should belong to some other, more significant class or object.

Consider that you already consider the map size in one other place (assigning random locations in the early stage of simFight). If you change your mind about the size, you have to edit two separate pieces of code. This is a potential cause of error which will only get worse as the code becomes larger and more complex.

So the size should be defined somewhere else, in one place. How, then, would Position find it? Well, I don't think Position should. Firstly, if it has to know where to look, that creates a dependency between this simple type and the more complex structure around it; if the structure had to change, Position would have to be rewritten to adapt to that, which is not good. The simple components of your code should be reusable without having to be rewritten. So knowing the size and limits of the map should be the responsibility of the object which creates positions.

If this seems odd to you, consider that you may decide to impose other limitations on location. For example, if you decide that only one fighter can occupy a location at once, or if you put obstacles on the map. You surely wouldn't rewrite Position to know about all of those. Its responsibility is purely to represent a point on the map, not to know anything about the map.

The size of the map really should be a parameter passed to simFight. In your current, simple code, simFight can take the responsibility of knowing the map size and imposing proper limits. That said, if your code is going to be any more complex, some new type should take over that responsibility. Consider a Map or Grid object - after all, your map surely is entitled to understand positions. simFight creates your map object with the right size, the map object creates position objects (and can be asked to return random map positions as well).

distance

Calculating the distance between two positions is not the job of a position. Distance is a property of a relationship between two objects, which is something your fight/map/grid knows about.

You might, as this code grows, also (or alternatively) calculate orthogonal distance; your fighters move orthogonally, after all. You might want to calculate the bearing of one object on the map compared to another, or find all map objects within range. You should not have to rewrite the Position class when these occur; they belong somewhere else (a map/game class or its companion object, for instance). Even if you don't add those, I think it helps show that the distance function is the concern of some other part of the hierarchy.

Style

Even in a small piece of code like this, it's worth creating a type alias for coordinates, something like this:

type Coordinate = Int;
class Position(val x: Coordinate, val y: Coordinate) {
}

This has two benefits:

1. If you decided to switch to Long or some other type, the amount of editing required is significantly reduced and the risk of errors (forgetting an edit somewhere) equally lessened.
2. It adds to the meaning and readability of code. Everywhere a variable, parameter or return type is deciared a Coordinate, the purpose is clarified. This lessens the need for comments and documentation and can even shorten the length of function or variable names.

An apply method in a companion object would provide a more idiomatic Scala way of creating new positions (Position(1,1) rather than new Position(1,1)). But I'm going to say more later about how positions should be created.

Adding a GameMap Trait

I've already stated that I think there should be a distinct object responsible for curating the game map and holding map-related functions like distance. This object could be passed (e.g as a constructor parameter) to objects which are placed on (or interact with) the map, so that they could call its methods. I'll explain that in more detail shortly but, as an example, Movable.right() should call something like a gamemap object's moveXByN method (passing +1 as a value, where left would pass -1).

Why a Trait

GameMap should be a trait, rather than a class. Classes which want to be placed on or interact with the map should take as input (e.g. to a constructor method) any class possessing the Map trait. This gives you great flexibility. Your current, simple code could just create an anonymous GameMap-based class inside simFight. Later, you could create more complex GameMap types - multiple different implementations - and drop them into the code without having to rewrite any Fighter code.

GameMap Solely Responsible for Positions

The gamemap object should be the only thing that can create positions. Requests for new positions (whether new, random ones or moves from one to another) should all go through this object. This allows you to hide the map implementation and change it (or introduce a new class with a radically different implementation) without any impact on the code of Movable.

If Movable movement methods all ask a gamemap object to move it by N and accept the result, then you can have a map which doesn't let them move past the edge of the map (or, alternatively, wraps them so they reappear from the opposite side) without movables having to be aware of the map size or even that it has edges.

To make that watertight (which is the safest and least fragile/error-prone course), you could make the Position constructor private, put Position into a package with GameMap and so on. But that's more complexity than we need to discuss here. The important thing to know is that you can only really change your map implementation at will if the map owns positions.

Implementation

Potential GameMap methods:

• makePosition(x: Coordinate, y: Coordinate): Position
• randomPosition() : Position
• moveXbyN(current: Position, n: Coordinate): Position
• moveYbyN(current: Position, n: Coordinate): Position
• distanceBetween(a: Position, b: Position)

There could be more but those cover what your code currently does. I don't think I need to tell you how you could create a concrete class which implements GameMap and applies your map size etc.

Not as complex as it looks

I've said quite a bit about this but it should actually be quite a simple trait to create. It will also simplify the code of both Movable and simFight, if you do it right.

Movable

I'm going to suggest splitting Movable, so that you have a simple trait which allows things to be placed on a map and a dependent type which allows such a thing to move.

Fighters

I have a few things to say about these. The first thing to say is that they probably should not be subclasses of 'Movable'

turn and simFight