Simple text-based RPG in Java

Question

I was looking for suggestions on how to improve the general flow of this code, as well as minimizing if / switch conditionals. Any suggestions are welcome however, as well as general game and game play suggestions (player scaling, enemy scaling, attack and enemy attack probabilities, which are handled in the dice class).

import java.util.*;
public class driver {
 static Scanner scan = new Scanner(System.in);
 static Random rand = new Random();
 static dice die = new dice();
 public static String playerName;
 public static int playerhp;
 public static int maxhp;
 public static int maxmana;
 public static int mana;
 public static int playermeleedmg;
 public static int xp;
 public static int enemyhp;
 public static int enemymeleedmg;
 public static int Level;
 public static String charclass;
 public static boolean fighting = false; //globals for player stats & enemy stats
 private static void printStats() {
 if(charclass.equals("mage")){
 System.out.println(playerName + "\nhp: " + playerhp + "\nmana: " + mana + "\ndamage: " + playermeleedmg + "\nxp: " + xp + "\n");
 }else{
 System.out.println(playerName + "\nhp: " + playerhp + "\ndamage: " + playermeleedmg + "\nxp: " + xp + "\n");
 }
 }
 private static void printEnemyStats() {
 System.out.println("Enemy "+"\nhp: " + enemyhp + "\ndmg: " + enemymeleedmg + "\n");
 }
 private static void buildWarrior() {
 charclass = "warrior";
 maxhp = 20;
 playerhp = 20;
 playermeleedmg = 4;
 xp = 0;
 Level = 1; 
 }
 private static void buildArcher() {
 charclass = "archer";
 maxhp = 14;
 playerhp = 14;
 playermeleedmg = 6;
 xp = 0;
 Level = 1;
 }
 private static void buildMage() {
 charclass = "mage";
 maxhp = 10;
 playerhp = 10;
 mana = 20;
 maxmana = 20;
 playermeleedmg = 2;
 xp = 0;
 Level = 1; // initializes globals according to class
 }
 private static void buildEnemy() {
 switch(Level){
 case 1:
 enemyhp = 9;
 enemymeleedmg = 1;
 break;
 case 2:
 enemyhp = 19;
 enemymeleedmg = 4;
 break;
 case 3:
 enemyhp = 24;
 enemymeleedmg = 6;
 break;
 case 4:
 enemyhp = 32;
 enemymeleedmg = 7;
 break;
 case 5:
 enemyhp = 40;
 enemymeleedmg = 9;
 break; //initializes enemy stats based on player level
 }
 }
 private static void fight() {
 String action;
 String spellAction = null;
 System.out.println("An enemy approaches");
 buildEnemy();
 fighting = true;
 while(fighting = true){
 System.out.println("Press 'a' to attack\nPress 'i' for info");
 if(charclass.equals("mage")){
 System.out.print("Press 's' for spells\n");
 }
 action = scan.nextLine();
 if(action.charAt(0) == 'a'){
 fighting = attack();
 if(fighting == false){
 switch(Level){
 case 1: 
 xp = xp + 4;
 break;
 case 2:
 xp = xp + 6;
 break;
 case 3:
 xp = xp + 9;
 break;
 case 4:
 xp = xp + 12;
 break;
 }
 System.out.println("You earned :" + xp + " xp");
 checkLevelUp();
 return;
 }
 enemyattack(); 
 }
 if(action.charAt(0) == 'i'){
 printStats();
 printEnemyStats();
 }
 if(action.charAt(0) == 's'){
 System.out.println("Press 'f' for fireball\nPress 'h' to heal\n");
 spellAction = scan.nextLine();
 if(spellAction.charAt(0) == 'f'){
 if(die.roll10() > 2){
 mana = mana - 10;
 if(mana <0){
 System.out.println("You don't have enough mana...");
 mana = mana + 10;
 }else{
 int k = die.roll10(); //randomly hurts 1-10
 System.out.println("You hit for " + k + " damage!");
 enemyhp = enemyhp - k;
 if(enemyhp <= 0){
 System.out.println("You Won!"); 
 switch(Level){
 case 1: 
 xp = xp + 4;
 break;
 case 2:
 xp = xp + 6;
 break;
 case 3:
 xp = xp + 9;
 break;
 case 4:
 xp = xp + 12;
 break;
 }
 System.out.println("You earned :" + xp + " xp");
 checkLevelUp();
 return;
 }
 enemyattack(); 
 }
 }
 else{
 System.out.println("You miss!");
 enemyattack();
 }
 }else
 if(spellAction.charAt(0) == 'h'){
 mana = mana - 8;
 if(mana <0){
 System.out.println("You don't have enough mana...");
 mana = mana + 8;
 }else{
 int x = die.roll10(); //randomly heals 1-8
 System.out.println("You heal your wounds...");
 System.out.println("+ " + x + " hp");
 playerhp = playerhp + x;
 if(playerhp>maxhp){
 playerhp = maxhp;
 }
 enemyattack(); 
 }
 }
 }
 }
 }
 private static void checkLevelUp() {
 if(xp >= 100 && Level == 4){
 System.out.println("Level 5!");
 Level = Level + 1;
 maxhp = maxhp + 25;
 playerhp = maxhp;
 if(charclass.equals("mage")){
 maxmana = maxmana + 7;
 mana = maxmana;
 }
 playermeleedmg = playermeleedmg + 3;
 printStats();
 }else
 if(xp >= 50 && Level == 3){
 System.out.println("Level 4!");
 Level = Level + 1;
 maxhp = maxhp + 20;
 playerhp = maxhp;
 if(charclass.equals("mage")){
 maxmana = maxmana + 7;
 mana = maxmana;
 }
 playermeleedmg = playermeleedmg + 2;
 printStats();
 }else
 if(xp >= 25 && Level == 2){
 System.out.println("Level 3!");
 Level = Level + 1;
 maxhp = maxhp + 10;
 playerhp = maxhp;
 if(charclass.equals("mage")){
 maxmana = maxmana + 7;
 mana = maxmana;
 }
 playermeleedmg = playermeleedmg + 2;
 printStats();
 }else
 if(xp >= 10 && Level == 1){
 System.out.println("Level 2!");
 Level = Level + 1;
 maxhp = maxhp + 5;
 playerhp = maxhp;
 if(charclass.equals("mage")){
 maxmana = maxmana + 7;
 mana = maxmana;
 }
 playermeleedmg = playermeleedmg + 1;
 printStats();
 }//increments player level and adds to stats with xp
 }
 private static void enemyattack() {
 if(die.roll6() > 2){
 System.out.println("Enemy hits!");
 playerhp = playerhp - enemymeleedmg;
 if(playerhp <= 0){
 gameover();
 System.exit(0);//game over if player health < 0
 }
 }else{
 System.out.println("Enemy Misses!");
 } 
 }
 private static boolean attack() {
 if(die.roll6() > 2){
 System.out.println("You hit!");
 enemyhp = enemyhp - playermeleedmg;
 if(enemyhp <= 0){
 System.out.println("You Won!"); //prints if enemy health < 0
 return false; 
 }
 }else{
 System.out.println("You miss!");
 }
 return true;
 }
 private static void gameover() {
 System.out.println(playerName + " Died!") ;
 System.out.println("GAME OVER!");
 System.exit(0); //terminates if lost
 return;
 }
 public static void main(String[] args) {
 String charclass;
 int num = 2;
 while(num > 1){
 System.out.println("Enter your Name: ");
 playerName = scan.nextLine();
 System.out.println("Choose your class: ");
 System.out.println("'w' for warrior");
 System.out.println("'a' for archer");
 System.out.println("'m' for mage");
 charclass = scan.nextLine();
 while(charclass.charAt(0) != 'w' && charclass.charAt(0) != 'a' && charclass.charAt(0) != 'm'){
 System.out.println("'w' for warrior");
 System.out.println("'a' for archer");
 System.out.println("'m' for mage");
 charclass = scan.nextLine();
 }
 if(charclass.charAt(0) == 'w'){
 buildWarrior();
 }
 if(charclass.charAt(0) == 'a'){
 buildArcher();
 }
 if(charclass.charAt(0) == 'm'){
 buildMage();
 }
 printStats();
 while(Level == 1){
 fight();
 }
 System.out.println("This area is clear... time to move on\n");
 while(Level == 2){
 fight();
 }
 System.out.println("This area is clear... time to move on\n");
 while(Level == 3){
 fight();
 }
 System.out.println("This area is clear... time to move on\n");
 while(Level == 4){
 fight();
 }
 System.out.println("This area is clear... time to move on\n");
 while(Level == 5){
 fight();
 }//keeps in area until levelUp
 }
 }
}
import java.util.*;
public class dice {
public int roll6(){
 Random rand = new Random();
 int n = rand.nextInt(7);
 while(n == 0){
 n = rand.nextInt(7);
 }//1-6
 return n;
}
public int roll10(){
 Random rand = new Random();
 int n = rand.nextInt(11);
 while(n == 0){
 n = rand.nextInt(11);
 }
 return n;
}//1-10
public int roll20(){
 Random rand = new Random();
 int n = rand.nextInt(21);
 while(n == 0){
 n = rand.nextInt(21);
 }//1-20
 return n;
}
}

• Marc-Andre: Too bad I'm at work, this is a really nice question! What do you use to code? Are you using an IDE or anything?
• Johnsonite: I used eclipse Luna for this project.
• Stephen Byrne: while(fighting = true)...shouldn't that be == ?
• Stephen S: Um... why is this a discussion? It's while(fighting)
• Brian J: @Mawg This is Java, not JavaScript. So there is no 'triple-equals' sign.

Answer 1

OOP

This task really begs for object oriented programming.

As a first step,

I suggest moving the maxhp, playerhp, playermeleedmg, xp, level fields to a dedicated class for players, and rewrite the build* methods. You could create a PlayerFactory with the build* methods that create and return warriors, archers, mages, enemies. All these different kind of characters could inherit from a common base Player class.

Rolling the dice

To get a random number x in the inclusive range of [1:6], instead of rand.nextInt(7) and skipping zeros in a while loop, you should use 1 + rand.nextInt(6) to get the same effect.

The roll6, roll10, roll20 methods all use the same logic. Just one roll method with a parameter could do the job:

public class dice {
 private final Random random = new Random();
 public int roll(int max) {
 return 1 + random.nextInt(max);
 }
 public int roll6() {
 return roll(6);
 }
 // and so on ...
}

As demonstrated in this example, you probably don't need a new Random instance before each roll.

Chaining conditions

These conditions are mutually exclusive, therefore they should be chained with else if rather than using multiple independent if statements:

if(charclass.charAt(0) == 'w'){
 buildWarrior();
}
if(charclass.charAt(0) == 'a'){
 buildArcher();
}
if(charclass.charAt(0) == 'm'){
 buildMage();
}

In this example a switch would be even better, so that charclass.charAt(0) will only be evaluated once.

Don't repeat yourself

There are near-duplicate lines at multiple places. For example the enemyattack and attack methods implement the same logic, with only minor differences in the details. Those details can be parameters, so that you can avoid copy-pasting code. Later when you need to update copy-pasted sections, it will be extremely annoying to make parallel changes at multiple locations, and it will be very error-prone too. Probably 99% of the time it's better to extract common logic and generalize than to copy-paste.

Formatting

You're using Eclipse Luna, it has a feature to reformat nicely the entire code. It's good to use that until it becomes a natural habit to type nicely.

• Johnsonite: Thanks for your feedback! I implemented a few changes you suggested and i'm always glad to simplify my code.
• gmatht: Wouldn't switch(charclass.charAt(0)){...} be even better than an else if chain? (particularly if it has a default: case for when they try to press q etc.)
• Mike Baxter: "you probably don't need a new Random instance before each roll" - I'd say you definitely shouldn't be creating new instances of Random! With that code if you call new Dice().roll() multiple times in the same millisecond then you will get the same result. Either make sure that the instance of Random is re-used or that you are never using more than one instance of the Dice object.
• searchengine27: @gmatht no it wouldn't. Basically, it boils down to how the compiler breaks down the switch statement. If all the values are sequential values minus one value, then yes the switch is more efficient. But in this case, w (119), a (97), and m (109) are not sequetial. If it was b (98), a (97), and d (100), for example (notice the gap between b, and d, then yes you would want to use a switch statement and it would be more efficient. What it is at the compiler level is lookupswitch (gap of more than 1, and less efficient ,basically an elseif) versus tableswitch (most efficient).
• searchengine27: Take a look at artima.com/underthehood/flowP.html for more explicit details about how it works under the hood. Also, if you use an IDE such as Eclipse, Eclipse will allow you to view the Java assembly after you create a .class file, so you can assemble some test cases yourself, and then open up your .class file in your IDE to see exactly how it treats it.

Answer 2

Some of your switch cases are unnecessarily long, repetitive and include lots of code.

You could easily replace them with indexed arrays, making it easier to read, shorter, and easier to expand with more values later.

For example, instead of

switch(Level){
 case 1:
 enemyhp = 9;
 enemymeleedmg = 1;
 break;
 case 2:
 enemyhp = 19;
 enemymeleedmg = 4;
 break;
 case 3:
 enemyhp = 24;
 enemymeleedmg = 6;
 break;
 case 4:
 enemyhp = 32;
 enemymeleedmg = 7;
 break;
 case 5:
 enemyhp = 40;
 enemymeleedmg = 9;
 break; //initializes enemy stats based on player level
 }

You could do

int[] enemyHpArray = {1, 9, 19, 24, 32, 40};
int[] enemyMeleedmgArray = {1, 1, 4, 6, 7, 9};
enemyhp = enemyHpArray[Level];
enemymeleedmg = enemyMeleedmgArray[Level];

Note that I put the value 1 at the index 0, just to avoid putting a 0 there. It looks like you wouldn't use the 0 index of the array, but in case you do by accident, it may reduce the risk of crashing the game due to hp/damage being unintentinally set to 0.

Another possibility is to put only your actual values {9, 19, 24, 32, 40} in the array and then index on enemyHpArray[Level-1]; , but I personally think that would be more confusing/less readable code.

https://stackoverflow.com/questions/1938101/how-to-initialize-an-array-in-java

• Johnsonite: Glad to hear an idea long switch cases get annoying. Thanks!
• Michael: "It looks like you wouldn't use the 0 index of the array, but in case you do by accident, it may reduce the risk of crashing the game" You want it to be fail-fast, not fail-safe. A better value would be -1.

Answer 3

Format

As pointed by others, the formatting could be arranged. Any decent IDE have a code formatter ;)

Also, classes name should start with an upper case letter ;)

Using enum when it makes sense

As you are coding in Java, you should replace the String charclass with an enum which would make more sense. Strings are for representing text, so, you should ask yourselves if being able to upperCase a class make sense ;) You could even ask if a special enum value can * cast spell * for example ;)

enum CharacterClass {
 GENERIC_RANDOM_WARRIOR(true, true), SINGER(false, false), BIG_SLEEPER(false, true);
 public final boolean canWieldBigAxes;
 public final boolean loveToSleep;
 private CharacterClass(final boolean canWieldBigAxes, final boolean loveToSleep) {
 this.canWieldBigAxes = canWieldBigAxes;
 this.loveToSleep = loveToSleep;
 }
}

Conditions for someone who can wield axes could then look like this :

if (charClass.canWieldBigAxes) {
 // things
}
// instead of
if (charClass.charAt(0) = 'w')

Easier to read and more logical, right ?

Random number

Your Dice class is neat but you should follow Janos advices :

• use nextInt(x) + 1
• make Random a field that is initialized just once (it also makes for better random number)

OOP

It may not be your primary goal but your code is sadly not OO.

If you want to make it more OO :

You should try to find things that "entities" in your code are sharing.

For example, both your enemy and the hero could be considered "Entity That Can Fight" (from nom on : BattleUnit) that have max HP, current HP, MinMeleeDamage, MaxMeleeDamage, a level, and, most importantly, can try to hit their opponent !

Thus you could have the following interface :

interface BattleUnit {
 int getLevel();
 void setLevel(final int level);
 int increaseLevel();
 boolean isDead();
 int getHP();
 int getMaxHP();
 int setHP(final int newHP);
 void damage(final int valueToSubstract);
 boolean attack(final BattleUnit target);
 int getMinMeleeDamage();
 int getMaxMeleeDamage();
}

Your hero would be defined as extending the BattleUnit interface (or whatever you called it) because as a mighty hero he is a "Entity That Can Fight" (or sometimes he'll be a "Entity That Can Fight And Cast Spell")... same goes for the various enemies because they all are BattleUnit deep down.

class Hero implements BattleUnit {
 private String name;
 private CharacterClass charClass;
 // some fields
 public boolean isDead() {
 return getHP() <= 0;
 }
 // blablabla moar code
 public boolean attack(final BattleUnit target) {
 if (die.roll6() > 2) {
 System.out.println("You hit!");
 target.damage(produceDamage());
 if (target.isDead()) {
 System.out.println("You Won!"); // prints if enemy health < 0
 return false;
 }
 } else {
 System.out.println("You miss!");
 }
 return true;
 }

Answer 4

Formatting

It may seem petty, but, as janos already pointed out, use a code formatter. (Ctrl-Shift-F on a vanilla eclipse is your friend). Stick to one style throughout a project.

Especially indentation can be seriously misleading (this makes me cringe ;-) ):

 }
 }else
 if(..){
 ..
 if(..){
 ..
 }else{
 ..
 ..
 if(..){
 ..
 }
 .. 
 }
 }
}

Correct indentation enables you to see class / subclass / method / block / nested block boundaries at a glance.

Externalize 'magic' numbers

You'll have a hard time if you ever want to change anything about the game mechanics. Define and use constants (or constant fields or enums) for values that won't change.

public static final int INITIAL_ARCHER_HP = 14;
private static void buildArcher() {
 ..
 playerhp = INITIAL_ARCHER_HP;
 ..
}

This way, everything is defined once and all definitions are in one place.

Code

• keep in mind that String.charAt() will give you unexpected results if you have to use exotic charsets (see this discussion ).
• consider a simple return rather than System.exit(0). It might be fine for a small standalone Java application, but it terminates the JVM rather abruptly, which in a multithreaded application, could be rather suprising to all other threads..

(btw: the return; after the System.exit(0); is never reached)

Answer 5

A couple of suggestions on how you could repeat yourself less.

As you observed, a while loop does the check first so the loop is not guaranteed to execute once or more.

System.out.println("'w' for warrior");
System.out.println("'a' for archer");
System.out.println("'m' for mage");
charclass = scan.nextLine();
while(charclass.charAt(0) != 'w' && charclass.charAt(0) != 'a' && charclass.charAt(0) != 'm'){
 System.out.println("'w' for warrior");
 System.out.println("'a' for archer");
 System.out.println("'m' for mage");
 charclass = scan.nextLine();
}

A do while loop, however, does the check at the end. This means it's guaranteed to execute at least once:

char charclass; // changed to char rather than string
// ...
do {
 System.out.println("'w' for warrior");
 System.out.println("'a' for archer");
 System.out.println("'m' for mage");
 charclass = scan.nextLine().charAt(0);
} while (charclass != 'w' && charclass != 'a' && charclass != 'm')

As you can see, I also changed the definition of the charclass variable.

---

You can also refactor this:

while(Level == 1){
 fight();
}
System.out.println("This area is clear... time to move on\n");
while(Level == 2){
 fight();
}
System.out.println("This area is clear... time to move on\n");
while(Level == 3){
 fight();
}
System.out.println("This area is clear... time to move on\n");
while(Level == 4){
 fight();
}
System.out.println("This area is clear... time to move on\n");
while(Level == 5){
 fight();
}

to:

for (int currentLevel = 1; currentLevel <= 5; currentLevel++) {
 while(Level == currentLevel){
 fight();
 }
 System.out.println("This area is clear... time to move on\n");
}

Answer 6

You could further generalize your player creation code by moving the logic of whether a specific player class is for that player class code into that specific class instance. For instance,

public MagePlayerClass extends AbstractPlayerClass {
 @Override 
 boolean canCreate(final String arg) {
 return "m".equalsIgnoreCase(arg);
 }
}

In this way you completely remove a huge if-then-else or switch statement and can have hundreds of classes that extend AbstractPlayerClass.

Player instances would then be created by an abstract factory method that uses those sub-classes of AbstractPlayerClass to create and initialize the Player instance.

Your player creation code would loop over all classes in a specific folder that extend the AbstractPlayerClass until one of them returns true from a call to canCreate("m");

Furthermore, you now have a super class to move all the common player class code.

• Michael: Those class names are horrible :( MagePlayer and Player would be better.
• Michael: Also, canCreate should probably be static - otherwise you need to create a mage in order to check whether you should have been able to create it.
• Rick Ryker: If it is static then you cannot use inheritance. You would have to use reflection on the class type to see if it extends the AbstractPlayerClass and then call its static method canCreate(String).
• Michael: You can't inheritance for that method . And don't even suggest anything as horrible as using reflection for this. Looking at this again, I think this should be implemented with a factory class. You pass in a char, you get out a Player.
• Rick Ryker: More likely if you had hundreds of subclasses in a folder all implementing the interface, you would have your factory class read all class files in the folder and load them, instantiating one of each and then mapping the subclass' self reported creation id ("M") to the subclass object (my.package.MageImpl) then let the instantiated subclass get garbage collected. Many frameworks work this way. A shortcut would be to have it drop a mapping text file. Then load it on start up and only instantiate classes for files not found in the text file. M=my.package.MageImpl