Concurrent hero vs monster program

Question

I have made a simple concurrent program. It represents a battle between a hero and a monster. I am beginner in concurrency so I just want to hear your opinion. What are bad and good sides of my code and how can I improve it?

Interface Attack

public interface Attack {
public void attack(Object s, int dmg);
}

Main class

public class Main {
public static void main(String[] args) {
 Monster monster = new Monster(50, "monster");
 Hero hero = new Hero (50, "hero");
 ThreadFight tf = new ThreadFight(hero, monster);
 Thread t1 = new Thread (new Runnable() {
 @Override
 public void run() {
 tf.hitHero();
 }
 });
 Thread t2 = new Thread (new Runnable() {
 @Override
 public void run() {
 tf.hitMonster();
 }
 });
 t1.start();
 t2.start();
}
}

Class Hero

public class Hero implements Attack{
private int hp;
private String name;
private Monster monster;
public Hero (int hp, String name) {
 this.hp = hp;
 this.name = name;
}
public int getHp() {
 return this.hp;
}
public String getName() {
 return this.name;
}
public void decreaseHp(int dmg) {
 this.hp = getHp() - dmg;
}
public void setMonster(Monster monster) {
 this.monster = monster;
}
@Override
public void attack(Object s, int dmg) {
 if (s instanceof Monster) {
 Monster m = (Monster) s;
 System.out.println("Monster hp: " + m.getHp());
 System.out.println("Hero dealt " + dmg + " damage");
 m.decreaseHp(dmg);
 System.out.println("Monster hp is now: " + m.getHp());
 System.out.println("--------------------------");
 }
}
}

Class Monster

public class Monster implements Attack{
private int hp;
private String name;
Hero hero;
public Monster (int hp, String name) {
 this.hp = hp;
 this.name = name;
}
public int getHp() {
 return this.hp;
}
public String getName() {
 return this.name;
}
public void decreaseHp(int dmg) {
 this.hp = getHp() - dmg;
}
public void setHero(Hero hero) {
 this.hero = hero;
}
@Override
public void attack(Object s, int dmg) {
 if (s instanceof Hero) {
 Hero h = (Hero) s;
 System.out.println("Hero hp: " + h.getHp());
 System.out.println("monster dealt " + dmg + " damage");
 h.decreaseHp(dmg);
 System.out.println("Hero hp is now: " + h.getHp());
 System.out.println("--------------------------");
 }
}
}

Class ThreadFight

public class ThreadFight{
Hero hero;
Monster monster;
Random random = new Random();
Object lock = new Object();
public ThreadFight(Hero hero, Monster monster) {
 this.hero = hero;
 this.monster = monster;
}
public void hitMonster() {
 while (monster.getHp() > 0 && hero.getHp() > 0) {
 synchronized (lock) {
 try {
 Thread.sleep(2000);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 hero.attack(monster, random.nextInt(20));
 }
 if (monster.getHp() < 0) {
 System.out.println("Hero won.");
 }
 }
}
public void hitHero() {
 while (hero.getHp() > 0 && monster.getHp() > 0) {
 synchronized (lock) {
 try {
 Thread.sleep(2000);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 monster.attack(hero, random.nextInt(20));
 }
 if (hero.getHp() < 0) {
 System.out.println("Monster won.");
 }
 }
}
}

Answer 1

One major problem with this code that I can see is that you need to consider what you are synchronizing, and why. I understand your main objective here is learning concurrency but I've also added some other suggestions you may or may not want to use.

Concurrency

There are a couple of issues with the way you are synchronizing things. In your case, the things you'd want to prevent happening are:

1. Interleaving on the players' attacks. This might happen if two separate threads try and damage the same enemy.
2. Preventing both the Hero and Monster 'winning' (killing each other). Note: you might not want to prevent this

The synchronized blocks will prevent any code with the same lock executing at the same time. Therefore you need to synchronize:

1. Modifications to health of self or enemy.
2. Checks of health of self or enemy.

You should not synchronize the Thread.sleep(); calls because each thread will have stop and wait for all the other ones to stop sleeping. So what does this look like for your code? Let's see:

public void hitHero() {
 boolean isFighting = true;
 while (isFighting) {
 try {
 Thread.sleep(2000);
 } catch (InterruptedException e) {
 e.printStackTrace();
 }
 synchronized (lock) {
 monster.attack(hero, random.nextInt(20));
 if (hero.getHp() < 0) System.out.println("Monster won.");
 isFighting = hero.getHp() > 0 && monster.getHp() > 0;
 }
 }
}

Note that I have used the boolean isFighting in order to move the checks to HP inside the synchronized block.

This solution will not prevent the two from killing each other if one dies while the other is waiting to attack. To prevent this, you'll need to add other checks inside the synchronized block.

Encapsulation

Consider the Monster and the Hero. They both share some common traits. They both have HP, a name, and can both attack something. Why not make a parent class (let's call it Fighter) that is not just an interface, but provides implementation for attack, and a hp + name:

public class Fighter {
 private int hp;
 private String name;
 public Fighter (int hp, String name) { ... }
 public int getHp() { ... }
 public String getName() { ... }
 public void decreaseHp(int dmg) { ... }
 public void attack(Fighter f, int dmg) {
 System.out.println(f.name + " hp: " + f.getHp());
 System.out.println(name + " dealt " + dmg + " damage");
 f.decreaseHp(dmg);
 System.out.println(f.name + " hp is now: " + f.getHp());
 System.out.println("--------------------------");
 }

The great thing about this is that you lose the need for the instanceof check, since you know you can attack any fighter you want. Now adding a new type of fighter is super easy:

public class Hero extends Fighter { 
 public Hero (int hp, String name) {
 super(hp, name);
 }
}

In addition to this, I would certainly consider making all the variables inside ThreadFight private, and creating + starting the two Threads inside ThreadFight's constructor (or inside a startFight() method).

Overall though, I think you've done a good job! You're running the threads correctly and nearly got the synchronization right. It can take a while to fully understand what's happening with multiple threads and locks. Though there are possible improvements the Monster and Hero were also built well. It's a nice little program and you could certainly expand upon it if you wanted to practice more complex threading situations.

• Imus: Great answer. Only one minor detail: With your generalised fighter class suggestion (which is great) you also suggest creating a new class for the Hero. You're technically correct but without adding anything extra to the class there is no need for it. Creating a hero can just as well be Fighter hero = new Fighter(50, "hero")
• Daniel Causebrook: @Imus You're right, but I kept the separate classes in there because I imagined that the asker might want to expand his program at some point to learn more complex threading. Maybe the Hero could do more damage or something similar.

Answer 2

I would add more comments. Like doc-strings to the functions and that classes, that just tends to be good practice as it increases readability of the code. Otherwise, as far as I can tell, you're code is pretty good.

• Imus: Could you give an explicit example of a comment that would improve the given code? The general concensus these days is to write self documenting code instead of comments. Only add a comment to explain why you did something in a certain way.
• user164538: I've just always had to put comments in whatever I have been coding to help explain what it is the code is doing, and sometimes why some things are done a certain way.