I'm trying to make a text-based game and I think I've got a good start. However, this code seems a little clunky and I'm pretty sure there is a better way of doing most things in the code. I am very new to coding.

package text.based.adventure;

import java.util.*;

public class TextBasedAdventure 

 public static String Name;
 public static int HP;
 public static int maxHP;
 public static int xp;
 public static int atk;
 public static int def;
 public static int lvl;
 public static int potion;
 static Random rand = new Random();
 static Scanner UI = new Scanner(System.in);
 public static int dam = rand.nextInt(6) + 1;
 public static int heal = rand.nextInt(6) + 1;

 public static void room2() 

 

 public static void StartRoom() throws InterruptedException 
 String input;

 System.out.println("The room is dark and gloomy, it reeks of dead corpses and rotten food,");
 System.out
 .println("You look behind you, the skeleton you recently killed and the damaged map are on the floor");
 System.out.println("the only choice no is to move forward");
 System.out.println("avail commands: forward, heal");
 input = UI.nextLine();
 if (input.equals("heal")) 
 potion = potion - 1;
 HP = maxHP;
 System.out.print("You are now at max HP, HP=" + HP);
 Thread.sleep(2000);
 StartRoom();
 else if (input.equals("forward")) 
 room2();
 else 
 System.out.print("Please Choose A Valid Command");
 Thread.sleep(2000);
 StartRoom();
 
 

 public static void combatskel() throws InterruptedException 
 int skeldam = rand.nextInt(3) + 1;
 String input;
 int hpskel;
 hpskel = 5;
 if (HP <= 0) 
 gameover();

 
 if (hpskel <= 0) 
 System.out.println("You Defeated A Skeleton");
 atk = atk + 1;
 def = def + 1;
 maxHP = maxHP + 1;

 System.out.format("You Have Leveled Up! atk is now %d, def is now %d, HP is now %d", atk, def, HP);
 System.out.println("");
 return;
 
 while (hpskel > 0) 
 input = UI.nextLine();
 if (input.equals("attack")) 
 hpskel = hpskel - dam - atk;
 HP = HP - skeldam;
 System.out.format("You Have Been Hit, Your HP is %d", HP);
 System.out.println("");
 combatskel();

 else if (input.equals("heal")) 
 if (potion <= 0) 
 System.out.println("You Do Not Have Enough Potions");
 combatskel();

 else 
 HP = HP + heal;
 System.out.println("You Have Been Healed");
 System.out.format("You Have Been Hit, Your HP is %d", HP);
 System.out.println("");
 HP = HP - skeldam;
 Thread.sleep(1200);
 System.out.format("You Have Been Hit, Your HP is %d", HP);
 System.out.println("");
 combatskel();
 
 
 
 

 public static void combatzombie() throws InterruptedException 
 System.out.println("You Have Encountered a zombie");
 int zombdam = rand.nextInt(3) + 2;
 String input;
 int hpzomb;
 hpzomb = 7;
 if (HP <= 0) 
 gameover();

 
 if (hpzomb <= 0) 
 System.out.println("You Defeated A Zombie");
 atk = atk + 1;
 def = def + 1;
 maxHP = maxHP + 1;
 System.out.format("You Have Leveled Up! atk is now %d, def is now %d, HP is now %d", atk, def, HP);
 System.out.println("");
 return;
 
 while (hpzomb > 0) 
 input = UI.nextLine();
 if (input.equals("attack")) 
 hpzomb = hpzomb - dam - atk;
 HP = HP - zombdam;
 System.out.format("You Have Been Hit, Your HP is %d", HP);
 System.out.println("");
 combatzombie();

 else if (input.equals("heal")) 
 if (potion <= 0) 
 System.out.println("You Do Not Have Enough Potions");
 combatzombie();

 else 
 HP = HP + heal;
 System.out.println("You Have Been Healed");
 System.out.format("You Have Been Hit, Your HP is %d", HP);
 System.out.println("");
 HP = HP - zombdam;
 Thread.sleep(1200);
 System.out.format("You Have Been Hit, Your HP is %d", HP);
 System.out.println("");
 combatzombie();
 
 
 
 

 public static void gameover() 
 System.out.println(Name + " Died!");
 System.out.println("GAME OVER!");
 System.exit(0); // terminates if lost
 

 /**
 * @param args
 * the command line arguments
 * @throws java.lang.InterruptedException
 */
 public static void main(String args) throws InterruptedException 
 maxHP = 20;
 potion = 3;

 System.out.println("Welcome To Dungeon Maze");
 System.out.println("Please Input Your Name ");
 Name = UI.nextLine();
 System.out.format("%s is a brave adventurer", Name);
 System.out.println("");
 Thread.sleep(3000);
 System.out.println("They were sent on a task to check on one of");
 Thread.sleep(3000);
 System.out.println("the sacred underground shrines of Port Nyanzaru,");
 Thread.sleep(3000);
 System.out.format("but when one of the skeletons knocked off the map %s had,", Name);
 System.out.println("");
 Thread.sleep(3000);
 System.out.println("they had no choice but to try and escape the dungeon");
 Thread.sleep(3000);
 System.out.println("ARE YOU READY BRAVE ADVENTURER");
 HP = maxHP;
 lvl = 1;
 def = 1;
 atk = 1;
 Thread.sleep(5000);
 StartRoom();
 

Τhanks for sharing your code!

From looking at your current code, it looks like it will quickly become difficult to maintain. This game looks like a prime candidate to refactor into different classes.

You said that you were new to programming, so you may not have much (if any) experience with creating your own classes.

Classes can be thought of as a blueprint for an object. You should create classes to represent the things in your game. Some potential classes that jump out to me are maybe a Game class to represent the game itself, a Player class to represent the player of the game, a Monster or 'Enemy' class to represent an enemy in your game. You currently have a Skeleton and a Zombie so both of those could be an instance of Monster instead of a collection of values that you need to manage yourself!

You could settle with just those, or you could go even further and have a Room class, an Item class (a potion could be an item) and so on.

Your TextBasedAdventure class is doing everything at the moment; it will quickly become difficult to manage. Separating it out into different classes will go a long way towards increasing readability and also maintainability.

Let's start with creating the Player class and see why it would be preferable over your current way of handling the player (that being)

public static String Name;
public static int HP;
public static int maxHP;
public static int xp;
public static int atk;
public static int def;
public static int lvl;
public static int potion;

You currently have just a bunch of different values that are all loose in your program. Let's move these into a Player class to encapsulate them.

public class Player 

 private int HP;
 private int maxHP;
 private int xp;
 private int atk;
 private int def;
 private int lvl;

 private int numPotions;

 public Player()
 HP = maxHP;
 lvl = 1;
 // ... any other initial values you want
 

 /* Getters & Setters */
 public int getHp()
 return HP;
 

 public void setHP(int hp)
 HP = hp;
 

 // .. any other ones you need

So we have a Player class now that holds all the values that are related to the Player of your game.

You can also provide some helper methods in your Player that could look like this.

public boolean isAlive()
 return HP > 0;

It's a lot more readable to see something like this

if(player.isAlive())
 // do stuff

over this

if(HP > 0)
 // do stuff

And how about another one

public void heal()
 if(numPotions > 0)
 numPotions--;
 HP = maxHP;
 

now we can use

player.heal();

instead of implementing it directly in the main method. This also has the advantage of allowing us to call this method any time it needs to be used, and also changing the implementation in one place if it needs to change, instead of finding and changing the logic all throughout your code.

A big problem I can see is your methods combatskel() and combatzombie(). These methods are almost identical, the only thing that changes is the text that is printed out and some numbers. This could be refactored to take an instance of a Monster as an argument.

First we need to make a Monster class. Looking at those 2 methods, let's take out all the variables that a Monster should have.

It looks like hp, damage, atk, def. I may be missing some but let's go with this for now.

We might end up with something like this

public class Monster 

 private int hp;
 private int damage;
 private int atk;
 private int def;

 public Monster(int hp, int damage, int atk, int def) 
 this.hp = hp;
 this.damage = damage;
 this.atk = atk;
 this.def = def;
 

 public int getHp() 
 return hp;
 

 public int getDamage() 
 return damage;
 

 public int getAtk() 
 return atk;
 

 public int getDef() 
 return def;
 

Now we have a lot of cross over between the Player class and the Monster class, so we could derive them both from a Creature class or something similar. But let's keep things simple for now and just have some code duplication.

Now that we have a Monster class, we could maybe make a method that looks like

public void combat(Monster monster)
 // do the fight logic

Now this monster could be a zombie, a skeleton, a ghost, orc, elf or however many new monsters you want to add to your game (you could put their information in a file and load in any number of them!)

Let's try and start writing the method out, copying the logic from your current implementation.

public void combat(Monster monster)
 System.out.println("You Have Encountered a " + monster.getName());
 int damage = monster.getDamage();

 if (!player.isAlive()) 
 gameover();
 

 if(!monster.isAlive())
 System.out.println("You Defeated A " + monster.getName());
 player.levelUp(); // doesn't exist - you could implement it yourself by placing the level up logic there.
 

 // and the rest...

This is obviously only the start, I won't completely refactor all of your code, but I hope this gives a good idea of how you can use objects and methods to reduce code duplication. Now you could have 1 method to handle combat with a monster instead of 1 x the number of monsters in your game!

Misc stuff

A lot of your methods throw an InterruptedException. This is implementation detail leaking out of your methods. There's no reason the calling code should have to handle these exceptions just because you want to use Thread.sleep() in your game. I would recommend something like this

public void wait(int numSeconds)
 try 
 Thread.sleep(numSeconds * 1000);
 catch (InterruptedException e) 
 // don't worry so much about this
 

Normally you should never do a try catch with an empty block, but in this scenario, it doesn't really matter if something messes up when sleeping, which will probably never happen. Now the rest of your code doesn't have to deal with the exceptions. You could use it like this

wait(3);

will wait for 3 seconds.

Don't use public variables. You use a lot of public variables in your main code, all variables should be private in 99% of cases. Some good examples of good use cases for public are Integer.MAX_VALUE and Math.PI. These are good examples because these are values that will never ever change. If you want to provide access to variables in other classes, create private fields and then create public getter methods.

Your variable names and method names should use camelCase. You adhere to this correctly in a lot of places, but variables like Name and StartGame violate this convention.

See the docs on naming convetions here

Hopefully this review was helpful for you!

• The first thing about your code that looked, as you put it, "clunky" to me was all those static variables in the beginning. The problem with this approach is that static variables are, in a way, global variables, but most of the variables you define at the beginning of your code are merely properties of the player. Now, as it is, this might not seem like a problem, because your class TextBasedAdventure is not inside any context whatsoever, so "global" and "property of the player" might not seem like irreconcilable characteristics, but it makes your code very inflexible, in case you want to modify your program in any way, be it reorganizing the code, or extending the functionality of the program, or refining the code that modifies the player's properties. In fact, even in the current state of your code, I can think of a disadvantage of making all these variables static: Unlike all the other variables, the player's name never changes, which means that the variable Name would lend itself to being declared final. However, this is not possible if the variable is static, because then its value would already have to be defined at compile time, meaning that it would be impossible for the user to input a name. So I suggest creating a Player class and making all these properties instance variables of Player.


• The code to level up or heal the player is littered over the methods StartRoom(), combatskel() and combatzombie(), which not only causes code duplication, but also defies the Single Responsibility Principle (in this case, this would mean that, for example, the method combatskel() is only responsible for fighting a skeleton, and not for the implementation details of leveling up the player. I suggest making levelUp and heal methods of their own, which you can then simply call from within StartRoom(), combatskel() and combatzombie() without having to repeat the code that actually does the leveling up. In case you do create a Player class, then it would seem natural to make heal() and levelUp() instance methods of Player.


• On to the method StartRoom(): You are calling this method from within itself, or, to put it another way, you are calling it recursively, which I don't think represents what the method is actually supposed to do. After all, the player doesn't re-enter the starting room, but it's just that the user is prompted again for input. So a better solution would be to put the code prompting the user for input into a loop that continues until the user enters "forward".


• 
  

  The methods combatskel() and combatzombie() are broken. The problem is that, again, you are calling these methods recursively, but in this case, this recursion causes a new skeleton/zombie to be spawned, because its health points are a local variable that is assigned at the beginning of the method. In fact, the if (hpskel <= 0) condition will never be true, because at this point, hpskel will always be five, no matter how deep in the recursion tree you are. This means that the method will end up calling itself until the player's health points reach 0, because before while (hpskel > 0) can be evaluated again after the first attack, we go one level deeper in the recursion tree, i.e. another skeleton/zombie is spawned, and so on. A solution to this problem would be to put everything in a loop instead, like I already suggested when reviewing the method StartRoom(). To illustrate what I mean, here a simplified version of your code:

  
  
  

  void combatzombie() 
   zombieHP = 5; //a zombie is spawned
   zombieHP--; //we attack the zombie, its HP are now 4
   combatzombie(); //this does not attack the zombie again, but spawns
   //a new zombie with 5 HP and attacks that zombie.
  
  

  
  
  

  Instead, we need this:

  
  
  

  void combatzombie() 
   zombieHP = 5; //a zombie is spawned
   while (zombieHP > 0) //we repeatedly attack this
   zombieHP--; //zombie until its HP reach 0,
   //no other zombie is spawned
  
  

  
  


• 
  

  Also, the methods combatskel() and combatzombie() contain a lot of common code. It would be better to put this common code in one place, and only put that which is specific for a skeleton or a zombie (i.e. the health points, the damage and the name of the monster) into the methods combatskel() and combatzombie(). For example, you could write a method combatMonster(String name, int health, int damage), and call this method from within combatskel() and combatzombie():

  
  
  

  void combatskel() 
   combatMonster("skeleton", 5, rand.nextInt(3) + 1);
  
  
  void combatzombie() 
   combatMonster("zombie", 7, rand.nextInt(3) + 2);
  
  

  
  



• Finally, a general remark about naming conventions: A lot of your code does not follow Java naming conventions. For example the field Name (fields conventionally start with lower case letters), the method StartRoom() (methods conventionally start with lower case letters as well), combatzombie() (a new word within a variable/method/class/whatever name should start with a capital letter, which is also known as "camel case").