Fight against Enemies (RPG in Java)

Question

I've written a program that simulates a fight between the player and an enemy to gain experience in the programming of such role-playing games. Above all, it was important to me to have good, object-oriented design. Criticism in this regard is expressly desired.

Explanation of the skills:

Fireball (needs 1 mana) - this is a skill that causes damage on an enemy. Nothing special.
Healing (needs 3 mana) - this skill heals the caster.
Knock Out (needs 2 mana) - this skill knocks the enemy out. He cant cast skills in this or in the next round. He also gets damage.
Poisoning (needs 3 mana) - the enemy gets damage and is poisoned in the next rounds. So he gets damage in the next rounds.

Sorry for my bad english. I will improve it :)

The program can be tested here

Here are my source files

Game.java

public class Game {
    private Player player;
    private NPC enemy;

    private int score;

    public Game() {
        player = new Player("Player", 20, 5, 5);
        enemy = new NPC("Enemy", 20, 5, 5);
    }

    public boolean fight() {
        UF.println(player.getName() + " fights against " + enemy.getName(), 1000);
        int rounds = 1;

        while(player.getLife() > 0 && enemy.getLife() > 0) {
            UF.println(" --- Round " + rounds + " --- \n", 1000);
            UF.println(player.getName() + " Stats", 0);
            UF.println("Life: " + UF.displayGraphical(player.getLife(), player.getLifeMax(), "#"), 0);
            UF.println("Mana: " + UF.displayGraphical(player.getMana(), player.getManaMax(), "@") + "\n", 1000);
            UF.println(enemy.getName() + " Stats", 0);
            UF.println("Life: " + UF.displayGraphical(enemy.getLife(), enemy.getLifeMax(), "#"), 0);
            UF.println("Mana: " + UF.displayGraphical(enemy.getMana(), enemy.getManaMax(), "@") + "\n", 1000);
            player.isPoisoned();
            if(player.getLife() <= 0) break;
            player.chooseSkill(enemy);
            if(enemy.getLife() <= 0 || player.getLife() <= 0) break;
            enemy.isPoisoned();
            if(enemy.getLife() <= 0) break;
            enemy.chooseSkill(player);
            rounds++;
        }

        if(player.getLife() > 0 && enemy.getLife() <= 0) {
            UF.println("YOU HAVE WON! YOU ARE NOW STRONGER", 2000);
            UF.println("PREPARE FOR THE NEXT FIGHT!\n", 2000);
            score++;
            enemy.getStronger(1.2);
            player.getStronger();
            return true;
        } else {
            UF.println("Game over!", 0);
            UF.println("You reached " + score + " Points. Gratulation!", 2000);
            return false;
        }
    }

    public void mainLoop() {
        while(fight());
    }

    public static void main(String[] args) {
        Game game = new Game();
        game.mainLoop();
    }
}

UF.java

// name stands for "useful functions"
public class UF {
    public static void println(String text, int time) {
        System.out.println(text);
        try {
            Thread.sleep(time);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }

    public static String displayGraphical(int value1, int value2, String symbol) {
        if(value2 < value1) {
            return "error";
        }

        String text = "[";
        int numberOfSymbols = 0;
        int numberOfWhiteSpace = 20;

        float filledOut = (float)value1 / (float)value2;
        numberOfSymbols = (int)(filledOut * 20);
        for(int i = 0; i < numberOfSymbols; i++) {
            text += symbol;
        }
        numberOfWhiteSpace -= numberOfSymbols;
        for(int i = 0; i < numberOfWhiteSpace; i++) {
            text += " ";
        }
        text += "] (" + value1 + " / " + value2 + ")";
        return text;
    }
}

Being.java

import java.util.Scanner;

public class Being {
    // basic values
    private String name;

    protected int lifeMax;
    protected int manaMax;
    protected int powerMax;

    protected int lifeActual;
    protected int manaActual;
    protected int powerActual;

    // skills
    protected Skill fireball;
    protected Skill knockOut;
    protected Skill healing;
    protected Skill poisoning;

    // state values
    protected boolean alive;
    protected boolean isKnockedOut;
    protected int[] isPoisoned;

    public Being(String name, int lifeMax, int manaMax, int powerMax) {
        this.name = name;
        this.lifeMax = lifeMax;
        this.manaMax = manaMax;
        this.powerMax = powerMax;

        this.lifeActual = lifeMax;
        this.manaActual = manaMax;
        this.powerActual = powerMax;

        // skills
        fireball = new Skill(this, "Fireball", 1, 1, 1);
        knockOut = new Skill(this, "Knock Out", 1, 0.5, 2);
        healing = new Skill(this, "Healing", 1, 2, 3);
        poisoning = new Skill(this, "Poisoning", 1, 0.2, 3);

        // state values
        alive = true;
        isKnockedOut = false;
        // how many rounds, how big is damage
        isPoisoned = new int[2];
    }

    @Override 
    public String toString() {
        String text = "";
        text += name + "\n";
        text += "Life:  " + lifeActual + " / " + lifeMax + "\n";
        text += "Mana:  " + manaActual + " / " + manaMax + "\n";
        text += "Power: " + powerMax + " / " + powerMax + "\n";
        return text; 
    }

    public String getName() {
        return name;
    }

    public int getLife() {
        return lifeActual;
    }

    public int getLifeMax() {
        return lifeMax;
    }

    public int getMana() {
        return manaActual;
    }

    public int getManaMax() {
        return manaMax;
    }

    public int getPower() {
        return powerActual;
    }

    public boolean isKnockedOut() {
        if(isKnockedOut) {
            UF.println(name + " is knocked out and can't cast skills this round\n", 1000);
            isKnockedOut = false;
            return true;
        } else {
            return false;
        }
    }

    public void isPoisoned() {
        if(isPoisoned[0] > 0) {
            UF.println(name + " is poisoned", 1000);
            sustainDamage(isPoisoned[1]);
            isPoisoned[0]--;
        }
    }

    public boolean isAlive() {
        return alive;
    }

    public void sustainDamage(int value) {
        if(value > 0) {
            lifeActual -= value;
            UF.println(name + " has gotten " + value + " damage\n", 1000);
        }

        if (lifeActual <= 0) {
            alive = false;
        }
    }

    public boolean useMana(int value) {
        if(value <= manaActual) {
            manaActual -= value;
            UF.println(name + " has used " + value + " mana.", 1000);
            return true;
        } else {
            UF.println(name + " has not enough mana.", 1000);
            return false;
        }
    }

    public void cureLife(int value) {
        int difference = lifeMax - lifeActual;
        if(value >= difference) {
            lifeActual = lifeMax;
            UF.println(name + " is fully healed.", 1000);
        } else {
            lifeActual += value;
            UF.println(name + " has healed " + value + " life points.", 1000);
        }
    }

    public void castFireball(Being enemy) {
        UF.println(name + " casts " + fireball.getName() + " on " + enemy.getName() + ".", 1000);
        if(useMana(fireball.getManaRequirement())) {
            enemy.sustainDamage(fireball.getValue());
        }
    }

    public void castHealing() {
        UF.println(name + " casts " + healing.getName() + ".", 1000);
        if(useMana(healing.getManaRequirement())) {
            cureLife(healing.getValue());
        }
    }

    public void castKnockOut(Being enemy) {
        UF.println(name + " casts " + knockOut.getName() + " on " + enemy.getName() + ".", 1000);
        if(useMana(knockOut.getManaRequirement())) {
            enemy.sustainDamage(knockOut.getValue());
            enemy.isKnockedOut = true;
            UF.println(enemy.getName() + " is knocked out.", 1000);
        }
    }

    public void castPoisoning(Being enemy) {
        UF.println(name + " casts " + poisoning.getName() + " on " + enemy.getName() + ".", 1000);
        if(useMana(poisoning.getManaRequirement())) {
            enemy.sustainDamage(powerActual / 2);
            enemy.isPoisoned[0] = poisoning.getLevel() + 2;
            enemy.isPoisoned[1] = poisoning.getValue();
        }
    }
}

Player.java

import java.util.Scanner;

public class Player extends Being {
    private static Scanner input = new Scanner(System.in);

    public Player(String name, int lifeMax, int manaMax, int powerMax) {
        super(name, lifeMax, manaMax, powerMax);
    }   

    public void chooseSkill(Being enemy) {
        if(!isKnockedOut()) {
            System.out.println("[1] Fireball");
            System.out.println("[2] Healing");
            System.out.println("[3] Knock Out");
            System.out.println("[4] Poisoning");
            System.out.println("[5] Give up");
            System.out.println("[*] Do nothing");
            System.out.print("Input: ");
            String command = input.next();

            if(command.equals("1")) {
                castFireball(enemy);
            } else if (command.equals("2")) {
                castHealing();
            } else if (command.equals("3")) {
                castKnockOut(enemy);
            } else if (command.equals("4")) {
                castPoisoning(enemy);
            } else if (command.equals("5")) {
                lifeActual = 0;
            }
        }
    }

    public void getStronger() {
        lifeMax += 5;
        manaMax += 2;
        powerMax += 1;

        lifeActual = lifeMax;
        manaActual = manaMax;
        powerActual = powerMax;

        isKnockedOut = false;
        isPoisoned[0] = 0;
        isPoisoned[1] = 0;
    }
}

NPC.java

import java.util.concurrent.ThreadLocalRandom;

public class NPC extends Being {
    public NPC(String name, int lifeMax, int manaMax, int powerMax) {
        super(name, lifeMax, manaMax, powerMax);
    }

    // this is actually my first KI, i am so proud
    public void chooseSkill(Being enemy) {
        if(isKnockedOut()) {
            return;
        }

        if (manaActual == 0) {
            lifeActual = 0;
            UF.println(getName() + " gives up because he has no mana.", 1000);
            return;
        }

        // decide for a skill
        int decision;
        while(true) {
            decision = ThreadLocalRandom.current().nextInt(0, 4);

            // i hope this is an accepted style, but i suspect it isnt
            if((decision == 1 && healing.getManaRequirement() > manaActual)
            || (decision == 1 && lifeMax == lifeActual)
            || (decision == 2 && knockOut.getManaRequirement() > manaActual)
            || (decision == 3 && poisoning.getManaRequirement() > manaActual)) {
                continue;
            } else {
                break;
            }
        }

        // cast the skill
        if(decision == 0) {
            castFireball(enemy);
        } else if (decision == 1) {
            castHealing();
        } else if (decision == 2) {
            castKnockOut(enemy);
        } else if (decision == 3) {
            castPoisoning(enemy);
        }
    }

    public void getStronger(double factor) {
        lifeMax *= factor;
        manaMax *= factor;
        manaMax += 3;
        powerMax += factor;

        lifeActual = lifeMax;
        manaActual = manaMax;
        powerActual = powerMax;

        isKnockedOut = false;
        isPoisoned[0] = 0;
        isPoisoned[1] = 0;
    }
}

Skill.java

public class Skill {
    private Being caster;
    private String name;
    private int level;
    private double value;
    private int manaRequirement;

    public Skill(Being caster, String name, int level, double value, int manaRequirement) {
        this.caster = caster;
        this.name = name;
        this.level = level;
        // means strength of the skill
        // multiplicated with level
        this.value = value;
        // also multiplicated with level
        this.manaRequirement = manaRequirement;
    }

    public String getName() {
        return name;
    }

    public int getValue() {
        return (int)(caster.getPower() * value * level);
    }

    public int getLevel() {
        return level;
    }

    public int getManaRequirement() {
        return manaRequirement * level;
    }
}

EDIT

I have two more questions that I would like to have answered.

Different languages

Let's say I want to publish my program in different languages. Accordingly, all output statements would have to be adjusted. What methodologies exist for such a project?

Graphic surface

Suppose I want to plot all data graphically. I would have to rewrite all classes. Is there an effective way to separate the processing of the data from the output of the data?

Answer 1

// name stands for "useful functions"
public class UF {

Just call it UsefulFunctions then. You probably will find an even better name, if you think about what kind of helper functions this class contains, but never abbreviate a class name like that. And most importantly, never make it depend on a comment to be understandable.

More idiomatic names might be HelperFunctions or UtilityFunctions, even better if you add what they are for. You can also omit the Functions, since it is kind of obvious, and the class represents the thing which is a Helper or Utility. For Example IOHelper.

As statet by Timothy Truckle more explicitly in a comment, the Helpers do not need to be static, and it actually hurts your attempt of good object oriented design. This is what I wanted to imply by "the class represents the thing ...", which means you better treat them as objects with a certain behaviour. So the classes are more than just a set of functions. That whole class is actually your Output, not just some UsefulFunctions.

Your Output should abstract how and what exactly is being written out, and have an interface with methods like printAllAliveBeingsStats(Collection<Being> beings) (which comes close to what the first 8 lines of the main loops are doing) instead of the whole code being put into the Game class.

---

NPC might be idiomatic enough in the context of an RPG, but I would still recommend naming it NonPlayerCharacter, and derive it from a Character. I highly advise against deriving from Being, or even having anything named Being, because that could basically be anything, depending on the interpretation. Better call it Creature, if that is what you intended it to represent.

---

Being has a boolean field alive, and a method isAlive(), which returns the variable's value. The value is set once the lifeActual is set to 0. A better implementation of isAlive() would be to just return lifeActual > 0. This reduces the complexity of your code and reduces the risk of introducing hard to find bugs.

---

alive and isKnockedOut are inconsistent names. Often boolean variables are named with just the adjective (alive and knockedOut), because they are attributes of the object, while the methods returning them are named isAlive and isKnockedOut (which your's are already), because they are questions about the object's state.

---

lifeActual and the other actual variables are a bit confusing. What other lifes can there be? Metaphoric ones? Better call it currentLife, because it is the current state of the object. Also, currentLife reads more naturally than lifeCurrent. The latter even sounds like a noun.

---

You have a lot of getters and setters. This breaks encapsulation, and is not object oriented. It is just slightly better than accessing the fields directly. While in some cases they makes sense, it is not good practice to use them for every field. Objects should only expose the information about themselves that they need to.

For example, instead of allowing the Skill to access the caster's power level, add the power level to the constructor parameters of the skill and let the caster inject its power level. Since this is the only reason you inject the caster into the skill, you can even remove the knowledge about the caster from the skill class.

---

        if(command.equals("1")) {
            castFireball(enemy);
        } else if (command.equals("2")) {
            castHealing();
        } else if (command.equals("3")) {
            castKnockOut(enemy);
        } else if (command.equals("4")) {
            castPoisoning(enemy);
        } else if (command.equals("5")) {
            lifeActual = 0;
        }

This would look much clearer as a switch statement. Note that this did not work with strings before Java 7, but you should be over that.

        switch (command) {
            case "1":
                castFireball(enemy);
                break;
            case "2":
                castHealing();
                break:
            // and so on
        }

---

isPoisoned = new int[2];

How can you have an array of being poisoned? I don't quite understand what these 2 values represent, but if they are different things related to being poisoned, better use two variables. Also, isSomething implies a boolean value, so better not use it for int values.

---

        System.out.println("[1] Fireball");
        System.out.println("[2] Healing");
        System.out.println("[3] Knock Out");
        System.out.println("[4] Poisoning");
        System.out.println("[5] Give up");
        System.out.println("[*] Do nothing");
        System.out.print("Input: ");

This can be done clearer by concatenating the output and do one single println call. Don't forget to add newlines (\n).

        System.out.println("[1] Fireball\n"
                         + "[2] Healing\n"
                         + "[3] Knock Out\n"
                         // ...
                         + "[*] Do nothing\n");

The compiler will concatenate the strings during compile time, so no need to worry about performance of string concatenation.

---

getStronger() looks like a getter. becomeStronger() might be better.

---

while(player.getLife() > 0 && enemy.getLife() > 0) {

Here you could use the isAlive() method, and thus hide the details of how "being alive" is implemented.

---

You have the int score in the Game class. Shouldn't that be part of the Player? The Game can't score by killing NPCs, and neither can the NPCs score by killing the Player. So the score is an attribute of the Player.

---

public static void println(String text, int time) {

lies about what it does. It suggests that it does almost the same as System.out.println, just maybe on a different output than the command line. A better name might be printAndwait(String text, int milliseconds).

---

Poisoning someone should not look like this: enemy.isPoisoned();. It looks like checking whether the enemy is currently poisoned or not. Better name it player.setPoisoned(bool poisoned), or just player.poison().

---

public static String displayGraphical(int value1, int value2, String symbol) {

What are these values? Better be more explicit with the parameter names. When calling the method, you should not need to read the method's body to know what parameters to pass. It should be obvious from the parameter names.

---

In displayGraphical of the class UF you use string concatenation a lot, in loops, and the method is called multiple times in the main loop. This is very inefficient, because every time you concatenate strings, a new string object is created, and after some time the old one will be deleted. Better use a StringBuilder, for better performance and memory efficiency.

---

Coding in English as a non-native speaker, a translation error has found its way into your code: Gratulation. The English word is Congratulations. The risk of this happening can be reduced by reading all of the strings from a file, having one file for each language if you want to support multiple languages, and letting someone translate the file for you who does not need to be a programmer, because the "strings" are not part of the program anymore.

Answer 2

The design needs overhaul. Specifically, the design of the Skill class (and concept)

Skills have properties and behaviors (=methods) that are related to the skill regardless of the caster. for example, each skill can be either casted on an enemy or on the caster himself. each skill has base power and mana consumption values, etc. Your code needs to clearly represent the properties and behaviors that define a skill regardless of the caster.

You have in your game four types of Skills. if you have a finite set of values of the same type, the best way to represent this is using enum. So, if we make Skill class an enum and remove the reference to Being, the design reflects the concept of Skill in the best way. For instance, initial values for the different skills are no longer defined in Being, but in Skill, the "name" property is redundant and I found a new attribute that is related to this type:

public enum Skill {

  FIREBALL(false, 1, 1).
  KNOCK_OUT(false, 0.5, 2).
  HEALING(true, 2, 3),
  POISONING(false, 0.2, 3);

  Skill(boolean castOnSelf, double value, int manaRequirement) {
     // assign to instance vars
  }

  private boolean castOnSelf;
  private double value;
  private int manaRequirement;
}    

This design also greatly simplifies the code for deciding what to cast (hint: oyu can get to an enum instance from its String name using valueOf())

now what about maxLevel? is it the same for all skill types or different?

Moving on: in your game, every Being has the same set of skills. that's fine. but every Being develops differently and this should be reflected by individual Skill instances together with the Being attributes that can modify a basic `Skill'

public class MySkill {
  private Skill skill;
  private int level;
  ...

So now, instead of individual variables, a Being has a Set of MySkill instances. (this means that MySkill instances should be considered equal if they have same skill) This design allows for iteration over the skill set of one Being and of course, different Beings can easily have different skill sets.

Moving on: as I understand it, the formula for the effect of skill casting involves the base value of the skill, related to the skill only, the power of the being, related to the being only, and the level of player in that skill. Perhaps in later version you will want to add defensive skills? also, in some RPG games, a random factor is added so that every skill casting can vary in effect. I would say that this complex logic merits its own CastingEngine that receives two Beings (attacker and target) and the skill that was chosen. If the CastingEngine is an interface, you could write different engines that evolve in complexity. the engine would apply all the above factors (including if the skill is casted on self....) and modify the Beings stats according to the outcome.

Answer 3

I'll target one specific point: magic numbers.

public Game() {
    player = new Player("Player", 20, 5, 5);
    enemy = new NPC("Enemy", 20, 5, 5);
}

When you look at this code thirty years from now, or even a few months, will you remember what these numbers are? Sure, you can look at the class and see what input it takes, but specifically saying what they mean makes the code much easier to read.

Focus on readability first. Making your code easy to understand is the best way to make sure it gets finished and is easily maintained.

Look at how much easier this is to read and understand; it's almost like natural language.

public Game() {
    final int lifeMax = 20;
    final int manaMax = 5;
    final int powerMax = 5;

    player = new Player("Player", lifeMax, manaMax, powerMax );
    enemy = new NPC("Enemy", lifeMax, manaMax, powerMax );
}

Answer 4

In addition to the answers already given, there's another problem with the Game class design.

First of all, the constructor creates the Player and NPC by itself, which leads to tight coupling: you could not replace any of the parameters used in creation of the Beings without changing the game class. Better: let the Game take its participants from the outside:

public Game(Player player, NPC enemy) {
    this.player = player;
    this.enemy = enemy;
}

...
in main:
    Player player = new Player("Player", 20, 5, 5);
    NPC enemy = new NPC("Enemy", 20, 5, 5);
    Game game = new Game(player, enemy);
    game.mainLoop();

Next step is: does the game really have to know who is a Player, and who is an NPC? You modeled both classes as being derieved from the common class Being, so maybe it should be possible to match any being vs any other? Without looking through the code en detail, you should strive for:

public class Game {
    private Being player1;
    private Being player2;

    ...

    public Game(Being player1, Being player2) {
        this.player1 = player1;
        this.player2 = player2;
    }
    ...
}