HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Battle game in one 900-line class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
battlelineonegameclass900

Problem

A little bit of background; I'm a high school student in my junior year and I attended a tech competition last year when I enrolled in my high school's CS I course. Below is what I submitted to the competition and even though I won in my category, I remember the judges telling me that I could do better by implementing more OOP design and seperate my program into serveral methods/classes.

I don't have much experience with this however, and we are mostly doing review over Java this year in my AP CS course than actually learning new things. Below is my code, and I'd be so thankful if I could get some advice on how to improve it and more specifically, implement methods/classes than having one giant, 900 line class.

```
import java.util.Scanner;
import java.util.Random;

public class Class1

{

@SuppressWarnings("unused")
public static void main(String[] args)

{

// Main objects

Scanner console = new Scanner(System.in);
Random rand = new Random();

// Game variables

String[] enemies = { "Kobold", "Kobold Warrior", "Kobold Archer", "Kobold Overseer" };
String[] shopItems = { "Silver Sword", "Steel Sword", "Iron Helmet", "Iron Chestplate", "Iron Boots", "Iron Gauntlets", "Steel Helmet", "Steel Chestplate", "Steel Boots", "Steel Gauntlets", "Illbane" };
String randomItem = null;

int enemyAttackDamage = 25;
int enemyHealth = 0;

// Boss Variables

String[] bossList = { "Red Drake" };

int redDrakeArmor = 20;
int redDrakeAttack = 75;
int redDrakeSpecialAttackValue = 200;

// Player variables

int playerHealth = 100;
int playerAttackDamage = 50;
int initialPlayerAttack = playerAttackDamage;
int playerArmorValue = 0;

int numHealthPotions = 5; // How many potions the player will start with.
int healthPotionEffect = 30; //

Solution

Naming

Names like Class1 send the message that your code isn't complete. Give classes / variables / functions names that describe what they are or what they're responsible for.

Warnings

Don't suppress warnings unless you have a really good reason to do so.

@SuppressWarnings("unused")


Having this on your main method is suppressing warnings about unused variables:

int buyStrengthPotion;
int buyHealthPotion;


And unused labels:

SHOP:
POTIONCHOICE:


As you're developing code you want to try to keep the amount of clutter to a minimum. That means not introducing new elements until you are ready for them and cleaning them up when you don't need them anymore.

Duplication

Keep an eye out for duplication in your code. It's a sign that there is scope to extract some functionality so that it can be reused. For example consider these lines:

System.out.println("\n\tWhat would you like to do?");

// Player options

System.out.println("\t1. Attack");
System.out.println("\t2. Drink health potion");
System.out.println("\t3. Run!");
System.out.println("\t4. Drink strength potion");


These lines essentially output a menu of options to the player (subsequent lines fetch the input and decide what to do). This is a prime candidate for extracting a method to perform a common task. For example a method getPlayerAction which displayed the menu and returned the selected option. You may want to consider using an Enum instead of an int to indicate the selected option. This will make the code easier to read.

You've then got two lines that you use to display the current state of a fight. This:

System.out.println("\tYour HP is: " + playerHealth);
System.out.println("\t" + enemy + "'s HP: " + enemyHealth);


Is almost the same as this:

System.out.println("\tYour HP is: " + playerHealth);
System.out.println("\t" + enemyBoss + "'s HP: " + redDrakeHealth);


Again, this suggests that there's scope for extracting a method here, that has parameters for the various variables (enemy name / health).

Classes

As well as extracting functionality into functions, you can also look at collecting data and functionality together in classes. Just looking at the line:

System.out.println("\t" + enemyBoss + "'s HP: " + redDrakeHealth);


You can see that there is information like the enemies name and the enemies health that seems like they are properties of the same thing. It could be that this points to an Enemy class, or perhaps a Mob / Creature class. A followup question might be, does the Player need its own class / is it a specialised version of the Mob class.

Drop Logic

Your drop logic seems a bit inverted:

if(rand.nextInt(100) < healthPotionDropChance)          // Health Potion drop chance; differs from enemy to enemy.
{
    if ("Kobold Archer".equals(enemy))
    {
        healthPotionDropChance = 55;
    }


You check the random number against the likelihood of a potion being dropped, then afterwards, only if the item has been dropped you adjust the likelihood based on who is being fought. It feels like the likelihood should be calculated before the chance of it having dropped is checked. Again, if the drop likelihood was an attribute of a Mob/Create class it could be calculated during construction of the class.

Let's go shopping

Items in the shop have attributes (name, cost, impact the item has on various attributes). If they were attributes on an Item class it would make it easier to simplify your functionality. This would make bugs less likely and easier to fix. As it stands for example, you're adjusting the players goldAmount after an item is purchased, however there is no check to make sure that the player has sufficient funds before going ahead with the purchase.

Bracing

Whilst the language supports if statements without braces, it can lead to bugs being introduced, particularly during maintenance / feature enhancement. Consider this section of code:

if (numStrengthPotions > 0)

playerAttackDamage = playerAttack * strengthPotionEffect;  // 2
numStrengthPotions--;                                      // 3


From the line grouping, it appears that lines 2 & 3 are supposed to be within the if block, however only line 2 is actually in the dependant on the if. Generally speaking it's better to always include the braces for if statements, even if initially you only need one line, that way when you introduce the second line you don't introduce a bug.

Magic Numbers

Some people really hate the use of raw/magic numbers in code. I'm generally OK with them when they make sense in context, or if there's no better name for the value. If there is a better name, use a constant, however for things like the armor effect of items this might not be necessary (a constant FIVE is no better than using 5 for example). That said, lets look at this code:

player.armorClass += 15 - 10;


It's unclear where 15 and 10 c

Code Snippets

@SuppressWarnings("unused")
int buyStrengthPotion;
int buyHealthPotion;
SHOP:
POTIONCHOICE:
System.out.println("\n\tWhat would you like to do?");

// Player options

System.out.println("\t1. Attack");
System.out.println("\t2. Drink health potion");
System.out.println("\t3. Run!");
System.out.println("\t4. Drink strength potion");
System.out.println("\tYour HP is: " + playerHealth);
System.out.println("\t" + enemy + "'s HP: " + enemyHealth);

Context

StackExchange Code Review Q#144601, answer score: 18

Revisions (0)

No revisions yet.