Fight simulator

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) 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 fo

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:

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

• 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 ju

StackExchange Code Review Q#97736, answer score: 4