patternjavaModerate
Battle game in one 900-line class
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; //
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
Warnings
Don't suppress warnings unless you have a really good reason to do so.
Having this on your
And unused labels:
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:
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
You've then got two lines that you use to display the current state of a fight. This:
Is almost the same as this:
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:
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
Drop Logic
Your drop logic seems a bit inverted:
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
Bracing
Whilst the language supports
From the line grouping, it appears that lines 2 & 3 are supposed to be within the
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
It's unclear where 15 and 10 c
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--; // 3From 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.