Beginner Project: Bunny City

Question

I am a Java programmer, and I just recently started learning Scala for fun. I found a group of projects here, and I tried to do the graduation excercise. The problem is, my code looks a lot like java, and I want to make this code more styled to fit scala, though I'm not sure what that entails. (Making it more functional?)

This is my main game class:

object Game {

  def main(args: Array[String]): Unit = {
    val list : ListBuffer[Bunny] = ListBuffer.fill(5)(BunnyFactory.getInitial)

    while(list.size != 0){

      var iterator = 0;
      //make sure we have one male for mating
      var males = 0;
      //keep track of females for mating
      var females : ListBuffer[Bunny] = new ListBuffer

      for(i <- list){
        i.update(list)

        if(i.dead){
              println(s"Bunny ${i.name} died at age ${i.age}!")
              list.remove(iterator)
        }
        else {
          if(i.gender == Gender.Male && i.adult){
            males += 1;
          }else if (i.gender == Gender.Female && i.adult){
              females += i;
          }
          println(i)
        }
        iterator += 1;
      }
      if(males >= 1){
        for(i <- females){

          val child = BunnyFactory.getChild(i);
          println(s"Bunny ${child.name} was born!")
          list += child
        }
      }
      println(s"There are ${list.size} bunnies!")

      Thread.sleep(500)
    }

  }
}

My Bunny class (I'm not sure if my variable naming follows conventions here):

class Bunny(inColor : Color.Value, inGender : Gender.Value, inAge : Int, inName : String, radioactive : Boolean){
    private val _color = inColor;
    private val _gender = inGender;
    private val _name = inName;
    private var _radioactive = radioactive;
    private var _age = inAge;

    def gender = _gender
    def age = _age
    def name = _name
    def adult : Boolean = {_age >= 2 && !(_radioactive)}
    def color  = _color

    def dead : Boolean = {
      if(_radioactive){
        _age > 50;
      }else{
        _age > 10;
      }
    }

    def infect : Unit = {_radioactive = true}

    def update(list : ListBuffer[Bunny]) : Unit = {
      _age += 1
      if(_radioactive){
         list((Math.random() * list.length).toInt).infect
      }
    }
    override def toString() : String = {
      return s"${_color} ${_gender} Bunny ${_name}, ${_age} years old, Radioactive: ${_radioactive}"
    }
}

Bunny Factory class:

object BunnyFactory {
    def getInitial : Bunny = {
      return new Bunny(
                    Color.getRandom,
                    Gender.getRandom,
                    0,
                    getName,
                    false)
    }
    def getChild(mom : Bunny) : Bunny = {
      val rand = (Math.random() * 100).toInt
      return new Bunny(mom.color, Gender.getRandom, 0, getName, rand <= 2)
    }
    val names : ListBuffer[String] = {
      val source = Source.fromFile("res/names.txt")
      val lines : List[String] = source.getLines.toList
      source.close
      lines.to[ListBuffer]
    }
    def getName : String = {
      val rand = (Math.random() * names.size).toInt
      return names(rand)
    }
}

Gender object:

object Gender extends Enumeration{
  type Gender = Value
  val Male, Female = Value

  def getRandom() : Gender = {
        val rand : Int = (Math.random() * values.size).toInt
        return this.apply(rand)
      }
}

Color object:

object Color extends Enumeration{
  type Color = Value
  val White, Brown, Black, Spotted = Value

  def getRandom() : Color = {
    val rand : Int = (Math.random() * values.size).toInt
    return this.apply(rand)
  }
}

Answer 1

Let me start with some suggestions to improve your code a little bit in regards of scala/functional style. I will start with by class with the points that catched my eyes first and i might edit my post later on and add additional points.

Class structure and constructors

Instead of going with

class Bunny(inColor : Color.Value, inGender : Gender.Value, inAge : Int, inName : String, radioactive : Boolean){
    private val _color = inColor;
    private val _gender = inGender;
    private val _name = inName;
    private var _radioactive = radioactive;
    private var _age = inAge;

you can write it more concise like this:

class Bunny(val color : Color.Value, val gender : Gender.Value, var age : Int, val name : String, var radioactive : Boolean){

No need to declare them with other names inside the class. Notice however, that public getters and setters (for vars) will be created automatically. Going private val color : Color.Value on the other hand will create a private getter.

Instead of creating the adult variable on the fly (def adult : Boolean = {_age >= 2 && !(_radioactive)}) it may be better to make the main constructor accept an adult variable and to create a second constructor (which calls the main constructor) that does the logic above. One important benifit is, that it will be more easy to test when you can pass in the adult variable as is.

The getChild method in your BunnyFactory class seems to belong to your Bunny class. You could consider making a class for male and female bunnies because they behave different although they are both bunnies. Traits could be a good option here to add a functionality only to the female bunny class for creating new child bunnies.

Naming

In your Bunny class you have a method update(list : ListBuffer[Bunny]) : Unit. I would call that different. Maybe infectNeighbours or similar.

In your BunnyFactory class you could call the getInitial method createRandomBunny or so. Because getInitial makes me think "huh? What does it do?". Same for getChild

In your main game class don't use list but rather existingBunnies or something like this.

Shortcuts

In the toString method of Bunny class you use return s"${_color} ${_gender} Bunny ${_name}, ${_age} years old, Radioactive: ${_radioactive}". The more scalaish way to do this is to omit the "return". It is not needed. The result of the last expression is used as return value anyways. Same goes for most of the other returns.

---

Now to your main game class. I personally prefer using list methods instead of the for comprehension in most cases. So instead of for(i <- list){ I would do a list.forEach. But first what do we want? For example, do we really want our already dead bunnies to infect other bunnies before we remove them from the list?

Here comes the "functional thinking" into to game (assuming our var list is called existingBunnies):

//lets first get a list of bunnies that are still alive
val livingBunnies = existingBunnies.filter{ bunny =>
    if (bunny.dead)
        println(s"Bunny ${bunny.name} is dead with an age of ${bunny.age}!")

    bunny.dead == false
}

//Now create a list of bunnies that are born new. That can be solved using not var but val in a more pure functional style but to me that will make it more complicated so i stayed with the if condition.
var newBornBunnies = List() //We have no new born bunnies by default

//But if there is at least one mal left, each female bunny will create a child
if(livingBunnies.count(_.gender == Gender.Male) >= 1) {
    newBornBunnies = livingBunnies.filter( _.adult && _.gender == Gender.Female ).map( _ createChild )
}

//We add the new born bonnies to the existing living ones
existingBunnies = livingBunnies ++ newBornBunnies

//At the end with our new list of existing bunnies we let them infect each other
existingBunnies forEach (_ infectNeighbours existingBunnies)

Answer 2

I wrote my own implementation, nearly from scratch. Hopefully, you can get some ideas on how to make your code more functional. I thought it would be simpler to write my version than pointing where I would have done things differently.

I separated the printing of events from the main code instead of having printlns scattered everywhere in the code (separation of concerns).

I made everything immutable. If a Bunny ages one year, a new object is created. However, keeping the age and radioactivity mutable might not be a bad idea.

A radioactive bunny should only infect a non-radioactive bunny. It adds some complications.

The spec is not well defined in general, so there might be many features which we implemented differently.

The main method:
(the main method is implicit since this object extends App)

import scala.util.Random
import Bunny._
import EventsDetection.findEvents

object BunniesEvolution extends App {
  val random = Random
  val nBunniesAtStart = 5

  var bunnies: List[Bunny] = List.fill(nBunniesAtStart)(generateInitialBunny)
  var doContinue = true

  while (doContinue) {
    val prevBunnies = bunnies

    bunnies = evolveOneYear(bunnies)

    print(bunnies, prevBunnies)

    if (bunnies.isEmpty) {
      println("\n\n\n  No more bunnies...")
      doContinue = false
    } else {
      println("Hit Enter to continue, or q + Enter to quit.\n")
      doContinue = readLine.isEmpty()
    }
  }

  /**
   * The spec is very unclear about the ordering of the events. For example, we assume here that
   * if a bunny reaches 10 years and dies, it can still reproduce in that past year
   */
  def evolveOneYear(bunnies: List[Bunny]): List[Bunny] = {
    val babies =
      if (bunnies.exists(isAdultMale)) bunnies.filter(isAdultFemale).map(generateFromMother)
      else Nil

    val mutatedBunnies = mutateSome(bunnies)
    val agedBunnies = mutatedBunnies.map(ageOneYear)
    val survivors = agedBunnies.filterNot(doesDie)
    babies ++ survivors
  }

  def generateInitialBunny = {
    val color = Color(random.nextInt(Color.values.size))
    val gender = if (random.nextDouble < probMale) Gender.Male else Gender.Female
    val age = 0
    val name = names(random.nextInt(names.size))
    val isMutant = random.nextDouble < probRadioactive
    Bunny(Bunny.generateUniqueId, color, gender, age, name, isMutant)
  }

  def generateFromMother(mother: Bunny) = {
    val gender = if (random.nextDouble < probMale) Gender.Male else Gender.Female
    val age = 0
    val name = names(random.nextInt(names.size))
    val isMutant = random.nextDouble < probRadioactive
    Bunny(Bunny.generateUniqueId, mother.color, gender, age, name, isMutant)
  }

  def mutateSome(bunnies: List[Bunny]): List[Bunny] = {
    val nonMutantIds = bunnies.filterNot(_.isMutant).map(_.id)
    val nMutants = bunnies.size - nonMutantIds.size
    val newMutantsIds = pickRandom(nonMutantIds, nMutants).toSet
    bunnies.map(b => if (newMutantsIds.contains(b.id)) mutate(b) else b)
  }

  // Yates-Fisher partial shuffle.  Recursive method.
  def pickRandom(l: List[Int], n: Int): List[Int] = {
    if (n <= 0 || l.isEmpty) Nil
    else {
      val (left, rightWithElement) = l.splitAt(random.nextInt(l.size))
      rightWithElement.head :: pickRandom(left ++ rightWithElement.tail, n - 1)
    }
  }

  def print(bunnies: List[Bunny], prevBunnies: List[Bunny]): Unit = {
    println("\nBunnies:")
    bunnies.sortBy(_.age).foreach(b => println(s"  ${b}"))
    val events = findEvents(bunnies, prevBunnies)
    println("\nEvents:")
    events.foreach(println)
    println()
  }

The Bunny class and utility methods:
(Note that I defined a Bunny.id since the names are not unique.)

import java.util.concurrent.atomic.AtomicInteger

/** From http://codereview.stackexchange.com/questions/52483 */
object Bunny {
  object Gender extends Enumeration {
    type Gender = Value
    val Male, Female = Value
  }

  object Color extends Enumeration {
    type Color = Value
    val White, Brown, Black, Spotted = Value
  }

  val names = "Bobo Boomo Carrotto Pakko Rondo Strato Ungo Yego Zero Strumgo Vargo Petro Toto".split(" ").toList

  val probMale = 0.5
  val probRadioactive = 0.02
  val adultAge = 2
  val normalDeathAge = 10
  val mutantDeathAge = 50

  def isAdult(b: Bunny) = b.age >= adultAge && !b.isMutant // crazy adult rule, from the spec
  def isAdultMale(b: Bunny) = isAdult(b) && b.gender == Gender.Male
  def isAdultFemale(b: Bunny) = isAdult(b) && b.gender == Gender.Female

  def doesDie(b: Bunny): Boolean = b.age >= (if (b.isMutant) mutantDeathAge else normalDeathAge)

  def ageOneYear(b: Bunny): Bunny = { Bunny(b.id, b.color, b.gender, (b.age + 1), b.name, b.isMutant) }

  def mutate(b: Bunny): Bunny = { Bunny(b.id, b.color, b.gender, b.age, b.name, true) }

  val idGenerator = new AtomicInteger(0)
  def generateUniqueId = idGenerator.incrementAndGet()

  // assumes the id is really unique for each Bunny
  def mapById(bs: List[Bunny]): Map[Int, Bunny] =
    bs.groupBy[Int](_.id).map { case (id, bunnies) => (id, bunnies.head) }
}
import Bunny._

// Immutable
case class Bunny(id: Int, color: Color.Value, gender: Gender.Value, age: Int, name: String, isMutant: Boolean)

The Events:

sealed trait Event
case class Birth(b: Bunny) extends Event { override def toString = s"Birth of $b" }
case class Death(b: Bunny) extends Event { override def toString = s"Death of $b" }
case class Mutation(b: Bunny) extends Event { override def toString = s"${b.name} became a mutant." }

object EventsDetection {
  import Bunny.mapById

  def findEvents(bunnies: List[Bunny], prevBunnies: List[Bunny]): List[Event] = {
    val prevBunniesById = mapById(prevBunnies)
    val bunniesById = mapById(bunnies)

    val deadIds = prevBunniesById.keySet diff bunniesById.keySet
    val deathEvents = deadIds.map(id => Death(prevBunniesById(id))).toList

    val newBornIds = bunniesById.keySet diff prevBunniesById.keySet
    val birthEvents = newBornIds.map(id => Birth(bunniesById(id))).toList

    val constantIds = bunniesById.keySet diff newBornIds
    val mutationIds = constantIds.filter(id => (bunniesById(id).isMutant != prevBunniesById(id).isMutant))
    val mutationEvents = mutationIds.map(id => Mutation(bunniesById(id)))

    deathEvents ++ birthEvents ++ mutationEvents
  }  
}