I created a game that allows you to battle an enemy until one is defeated. It's my first attempt at writing a game like this in Java and I have been using it as a means to practice OOP and other topics as I learn them.

The current move choices each character has are a basic attack, throw ice crystal(freeze target for 2 turns), drink potion(restore 15 health), and throw fireball(guaranteed 20 damage).

I am having a hard time figuring out what kinds of additions I can add from here or how I can improve/ build upon my code in a way that I understand still being fairly new to the language.

import java.util.Random;
import javax.swing.JFrame;
import javax.swing.JOptionPane;

public class GameFile extends JFrame

 public static void main(String args)
 
 int playAgain;
 Random rng = new Random();
 do
 
 GameCharacter player = generatePlayer();
 GameCharacter enemy = generateEnemy();
 int playerFrozen = 0;
 int enemyFrozen = 0;
 do
 
 //player's turn
 if (player.isFrozenStatus() == false)
 
 int choice = menuSelect(player);
 if (choice == 0)
 basicAttack(player, enemy);
 else if (choice == 1)
 drinkPotion(player);
 else if (choice == 2)
 useFireBall(player, enemy);
 else if (choice == 3)
 useIceCrystal(player, enemy);
 else
 System.exit(0);
 
 else
 
 playerFrozen += 1;
 if (playerFrozen % 2 == 0)
 player.setFrozenStatus(false);
 

 //enemy's turn
 if (enemy.getHealthPoints() > 0 && !enemy.isFrozenStatus())
 
 int enemyChoice = rng.nextInt(4);
 if ((enemy.getHealthPoints() <= 10) && (enemy.getPotions() > 0))
 drinkPotion(enemy);
 else if (enemyChoice == 0 && enemy.getFireBalls() > 0)
 useFireBall(enemy, player);
 else if (enemyChoice == 1 && enemy.getIceCrystals() > 0 && !player.isFrozenStatus())
 useIceCrystal(enemy, player);
 else
 basicAttack(enemy, player);
 
 else if (enemy.getHealthPoints() > 0)
 
 enemyFrozen += 1;
 if (enemyFrozen % 2 == 0)
 enemy.setFrozenStatus(false);
 
 while (player.getHealthPoints() > 0 && enemy.getHealthPoints() > 0);

 playAgain = displayWinner(player, enemy);
 while (playAgain == 0);


// OTHER METHODS

public static GameCharacter generateEnemy()

 GameCharacter enemy = new GameCharacter(10, "the Evil Wizard", CharacterType.ENEMY);
 return enemy;


public static GameCharacter generatePlayer()

 String name = JOptionPane.showInputDialog("Welcome! What is your name?");
 if (name == null)
 System.exit(0);
 GameCharacter player = new GameCharacter(10, name, CharacterType.PLAYER);
 simpleMessage("Hi " + player.getName() + "! Get ready to battle your opponent!");
 return player;


public static void basicAttack(GameCharacter attacker, GameCharacter target)

 attacker.attack(target);
 simpleMessage((attacker.getType() == CharacterType.ENEMY
 ? attacker.getName() + " attacked you! You have " + target.getHealthPoints() + " health left."
 : "You attacked " + target.getName() + "! They have " + target.getHealthPoints() + " health left."));


public static void useIceCrystal(GameCharacter attacker, GameCharacter target)

 if (attacker.throwIceCrystals(target))
 
 target.setFrozenStatus(true);
 simpleMessage((attacker.getType() == CharacterType.ENEMY
 ? attacker.getName() + " threw an Ice Crystal!nYou are frozen for the next 2 rounds!"
 : "You threw an Ice Crystal!n" + target.getName() + " is frozen for the next 2 rounds!"));
 
 else if (attacker.getType() == CharacterType.PLAYER)
 simpleMessage("You are out of Ice Crystals....");


public static void useFireBall(GameCharacter attacker, GameCharacter target)

 if (attacker.throwFireBall(target))
 simpleMessage((attacker.getType() == CharacterType.ENEMY
 ? attacker.getName() + " threw a FireBall at you! You have " + target.getHealthPoints() + " health left."
 : "You threw a FireBall at " + target.getName() + "! They have " + target.getHealthPoints() + " health left."));
 else if (attacker.getType() == CharacterType.PLAYER)
 simpleMessage("You are out of FireBalls...");



public static void drinkPotion(GameCharacter drinker)

 if (drinker.usePotion())
 simpleMessage((drinker.getType() == CharacterType.ENEMY
 ? drinker.getName() + " used a potion! They have " + drinker.getHealthPoints() + " health."
 : "You used a potion! You have " + drinker.getHealthPoints() + " health."));
 else if (drinker.getType() == CharacterType.PLAYER)
 simpleMessage("You are out of potions!");


public static int displayWinner(GameCharacter player, GameCharacter enemy)

 if (player.getHealthPoints() <= 0)
 JOptionPane.showMessageDialog(null, "Sorry, you lose!n" + enemy.getName() + " has defeated you...");
 else
 JOptionPane.showMessageDialog(null, "Great job " + player.getName() + ", you win!" + "nYou have defeated " + enemy.getName() + "!");
 return JOptionPane.showConfirmDialog(null, "Would you like to play again?", " ", JOptionPane.YES_NO_OPTION);


public static int menuSelect(GameCharacter player)

 String moveChoices = "Attack", "Use Potion(" + player.getPotions() + ")", "Throw FireBall(" + player.getFireBalls() + ")",
 "Throw Ice Crystal(" + player.getIceCrystals() + ")" ;
 int choice = JOptionPane.showOptionDialog(null, "What would you like to do?", " ", JOptionPane.DEFAULT_OPTION, JOptionPane.PLAIN_MESSAGE,
 null, moveChoices, moveChoices[0]);
 return choice;


public static void simpleMessage(String boxMessage)

 if (JOptionPane.showConfirmDialog(null, boxMessage, " ", JOptionPane.OK_CANCEL_OPTION, JOptionPane.PLAIN_MESSAGE) != 0)
 System.exit(0);

Thanks for sharing your code!

Timothy Truckle already has a good review up but I'll try and add some more to it.

One thing I always like to think about is what would need to change if this project got a lot bigger

Currently, both the user and the enemy only have 4 different options, this is all well and good when the project scope is fairly small, but what happens when you start adding in ThunderBolt, PoisonCloud, Inferno etc. Do you go add a corresponding method in any relevant class and then add a a new condition in multiple parts of the code? This would need to happen given the current implementation, and would be very unmaintainable and also very error prone.

What would be nice to see is a single method which will handle the using of any arbitrary Ability. There are already several candidates for an Ability subclass. Those being, Fireball, Potion and IceCrystal. I would also even throw Attack in here as a possible option.

And instead of basicAttack, useFireBall, drinkPotion methods, we could have something like this.

GameCharacter player = ...;
GameCharacter target = ...;
player.useAbility(new Fireball(), target); 
player.useAbility(new Heal(50), player); // self heal
player.useAbility(new PoisonCloud(), target);

Now of course we need to somehow make this work!

public void useAbility(Ability ability, GameCharacter target)
 target.dealDamage(ability.spellPower()); // we're not calling target.setHp()

 for(StatusEffect effect : ability.statusEffects())
 effect.apply(target); // Burn, Poisoned, Regeneration etc.
 

 // the ability could also be a friendly one
 if(ability.isBuff())
 target.applyBuff(ability.getBuff());
 

Each Ability would have it's own attributes and methods to achieve the desired result. This would effectively eliminate the need for a new method per new skill, instead you would create a new subclass of Ability and just pass it to this method!

We've actually introduced a few new objects here we could possibly use. We have a StatusEffect class. At the moment, Frozen is the only status effect that's currently in your game. Think about how much would need to change if we wanted to change how your freeze effect works, everything that uses it would need to change. Instead we could encapsulate that in a StatusEffect class or enum. This would also allow us to easily create new ones like Burn, Regenerate, AutoRevive etc. One possible way this could be implemented is simply adding a Collection of status effects to the GameCharacter class.

public class GameCharacter 

 private Collection<StatusEffect> statusEffects = new HashSet<>(); 

 // ... constructors/rest of class

 public void addStatusEffect(StatusEffect se)
 statusEffects.add(se);
 

 public void startOfTurn()
 for(StatusEffect se : statusEffects)
 se.apply(this); // heal with regen, take damage with burn, prevent turn with freeze etc.
 
 

This line of code gets repeated in a lot of places

gameCharacter.getHealthPoints() > 0;

this is a violation of the Tell don't ask principle. Here, you are asking the object to tell you what its health points are, so that you can perform some calculation with that result. Instead, you should tell the object what you want to know. For example

gameCharacter.isAlive();

It doesn't look like a big a difference, after all it's still a single line of code, but let's image you introduce a new mechanic in the game. A StatusEffect that prevents death for 3 turns even if you're below 0hp. That seems like something that might be in an RPG. If you want to do this now, you'll need to go through every occurrence of checking for this > 0 and add an additional condition, this would be very inconvenient and also again, very error prone.

with an isAlive method, we could do it like this

// GameCharacter class
public boolean isAlive() this.hp > 0;

(this example assumes that the .equals and .hashCode in the HolyShield class will consider all instances of HolyShield to be equal)

now in our calling code, we don't care how the GameCharacter is alive, all we care about is that they are!

There is a similar issue with your checking if a gameCharacter is frozen. You currently keep track of this state with integers that are outside of the GameCharacter class. This state should be handled inside the GameCharacter class (or any subclasses).

Currently you have this

else if (enemy.getHealthPoints() > 0)

 enemyFrozen += 1;
 if (enemyFrozen % 2 == 0)
 enemy.setFrozenStatus(false);

I would prefer to see something like

else if (enemy.isAlive())
 enemy.chill(); // may freeze them, internally this has the enemyFrozen int

Hopefully this review was helpful for you, keep it up!

Welcome to Code Review SE!

Thanks for sharing your code.
It looks quite good for a first attempt.

OOP

OOP doesn't mean to "split up" code into random classes.

The ultimate goal of OOP is to reduce code duplication, improve readability and support reuse as well as extending the code.

Doing OOP means that you follow certain principles which are (among others):

• information hiding / encapsulation


• single responsibility


• separation of concerns


• KISS (Keep it simple (and) stupid.)


• DRY (Don't repeat yourself.)


• "Tell! Don't ask."


• Law of demeter ("Don't talk to strangers!")

Inheritance

In OOP we inherit from a super class if we extend its behavior. This is: we override a method to do something more and/or something different then the same method in the super class.

Your class extends JFrame but does not change a JFrames behavior.
It don't even access any method of JFrame. So your class should not even know that there is a JFrame class out there.

Tell, don't ask!

At certain places your code uses some information out of the GameCharacter object to calculate some information on it. E.g.:

 if (attacker.throwFireBall(target))
 simpleMessage((attacker.getType() == CharacterType.ENEMY
 ? attacker.getName() + " threw a FireBall at you! You have " + target.getHealthPoints()
 + " health left."
 : "You threw a FireBall at " + target.getName() + "! They have " + target.getHealthPoints()
 + " health left."));
 else if (attacker.getType() == CharacterType.PLAYER)
 simpleMessage("You are out of FireBalls...");

You should better do this Calculation in the GameCharacter class where you have all the information at hand, so all of this code should be in the method throwFireBall(target) in that class.

replace branching with polymorphism

Your class GameCharacter holds a property of type CharacterType and you select different behavior based on this property. This is a sign that you're missing a class hierarchy.

You should have two more classes Player and Enemy that extend GameCharacter which are holding the special behavior of either one:

abstract class GameCharacter
 public abstract void throwFireBallAt(GameCharacter target);


class Player extends GameCharacter
 @override
 public void throwFireBallAt(GameCharacter target)
 if (hasMoreFireballs)
 target.defeatFireball();
 simpleMessage("You threw a FireBall at " + target.getName() + "! They have " + target.getHealthPoints() + " health left.");
 else 
 simpleMessage("You are out of FireBalls...");
 
 

class Enemy extends GameCharacter{
 @override
 public void throwFireBallAt(GameCharacter target)
 if (hasMoreFireballs) 
 target.defeatFireball();
 simpleMessage(attacker.getName() + " threw a FireBall at you! You have " + target.getHealthPoints() + " health left.");
 
 

If for some reason you get another type of GameCharacter you simply create a new child class of GameCharacter and modify its behavior in that new class file instead of skimming all over your existing code looking out for places to add the new behavior.