# Battle game simulation seems to have too much code

Can someone tell me if my code is too long for what it does?  Please be nice — I just started trying to learn a couple days ago.

package Tests;

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

public class DuelMain {

public static void main(String[] args) {
Random battle = new Random();
@SuppressWarnings("resource")
Scanner input = new Scanner(System.in);

int hero, match, restart;

Duel valon = new Duel();
Duel rintar = new Duel();
Duel zersious = new Duel();
Duel balrock = new Duel();
Duel hawkeye = new Duel();
Duel yusef = new Duel();

valon.attack = 6;
valon.defense = 0;
valon.health = 19;

rintar.attack = 8;
rintar.defense = 1;
rintar.health = 16;

zersious.attack = 5;
zersious.defense = 2;
zersious.health = 18;

balrock.attack = 10;
balrock.defense =0;
balrock.health = 15;

hawkeye.attack = 7;
hawkeye.defense = 1;
hawkeye.health = 17;

yusef.attack = 13;
yusef.defense = 2;
yusef.health = 10;

System.out.println("Press 1 for the Mage Lord Valon             Press 2 for the Warrior Rintar");
System.out.println("Attack      6               Attack      8");
System.out.println("Defense     0               Defense     1");
System.out.println("Health      19              Health      16");
System.out.println("");
System.out.println("");
System.out.println("Press 3 for the Paladin Prince Zersious     Press 4 for the Orc Balrock");
System.out.println("Attack      5               Attack      10");
System.out.println("Defense     2               Defense     0");
System.out.println("Health      18              Health      15");
System.out.println("");
System.out.println("");
System.out.println("Press 5 for the Elf Hawkeye         Press 6 for the Dragon Summoner Yusef");
System.out.println("Attack      7               Attack      13");
System.out.println("Defense     1               Defense     2");
System.out.println("Health      17              Health      10");
hero = input.nextInt();
if(hero == 1){System.out.println("You have chosen Lord Valon!");
for(hero = 1;;){
System.out.println("");
match = 1+battle.nextInt(5);
if(match == 1){
System.out.println("Rintar!");
System.out.println("");
valon.attack1 = 8;
valon.defense1 = 1;
valon.health1 = 16;
valon.calculateWinner();
}else if(match == 2){
System.out.println("Prince Zersious!");
System.out.println("");
valon.attack1 = 5;
valon.defense1 = 2;
valon.health1 = 18;
valon.calculateWinner();
}else if(match == 3){
System.out.println("Balrock!");
System.out.println("");
valon.attack1 = 10;
valon.defense1 = 0;
valon.health1 = 15;
valon.calculateWinner();
}else if(match == 4){
System.out.println("Hawkeye!");
System.out.println("");
valon.attack1 = 7;
valon.defense1 = 1;
valon.health1 = 17;
valon.calculateWinner();
}else if(match == 5){
System.out.println("Yusef!");
System.out.println("");
valon.attack1 = 13;
valon.defense1 = 2;
valon.health1 = 10;
valon.calculateWinner();}
System.out.println("");
System.out.println("Fight again?");
System.out.println("1 for Yes");
System.out.println("2 for No");
restart = input.nextInt();
if(restart == 1){
System.out.println("");
}else if(restart == 2){
System.out.println("Thank you for playing!");
break;}
}
}
if(hero == 2){System.out.println("You have chosen Rintar!");
for(hero = 2;;){
System.out.println("");
match = 1+battle.nextInt(5);
if(match == 1){
System.out.println("Lord Valon!");
System.out.println("");
rintar.attack1 = 6;
rintar.defense1 = 0;
rintar.health1 = 19;
rintar.calculateWinner();
}else if(match == 2){
System.out.println("Prince Zersious!");
System.out.println("");
rintar.attack1 = 5;
rintar.defense1 = 2;
rintar.health1 = 18;
rintar.calculateWinner();
}else if(match == 3){
System.out.println("Balrock!");
System.out.println("");
rintar.attack1 = 10;
rintar.defense1 = 0;
rintar.health1 = 15;
rintar.calculateWinner();
}else if(match == 4){
System.out.println("Hawkeye!");
System.out.println("");
rintar.attack1 = 7;
rintar.defense1 = 1;
rintar.health1 = 17;
rintar.calculateWinner();
}else if(match == 5){
System.out.println("Yusef!");
System.out.println("");
rintar.attack1 = 13;
rintar.defense1 = 2;
rintar.health1 = 10;
rintar.calculateWinner();}
System.out.println("");
System.out.println("Fight again?");
System.out.println("1 for Yes");
System.out.println("2 for No");
restart = input.nextInt();
if(restart == 1){
System.out.println("");
}else if(restart == 2){
System.out.println("Thank you for playing!");
break;}
}
}
if(hero == 3){System.out.println("You have chosen Prince Zersious!");
for(hero = 3;;){
System.out.println("");
match = 1+battle.nextInt(5);
if(match == 1){
System.out.println("Lord Valon!");
System.out.println("");
zersious.attack1 = 6;
zersious.defense1 = 0;
zersious.health1 = 19;
zersious.calculateWinner();
}else if(match == 2){
System.out.println("Rintar!");
System.out.println("");
zersious.attack1 = 8;
zersious.defense1 = 1;
zersious.health1 = 16;
zersious.calculateWinner();
}else if(match == 3){
System.out.println("Balrock!");
System.out.println("");
zersious.attack1 = 10;
zersious.defense1 = 0;
zersious.health1 = 15;
zersious.calculateWinner();
}else if(match == 4){
System.out.println("Hawkeye!");
System.out.println("");
zersious.attack1 = 7;
zersious.defense1 = 1;
zersious.health1 = 17;
zersious.calculateWinner();
}else if(match == 5){
System.out.println("Yusef!");
System.out.println("");
zersious.attack1 = 13;
zersious.defense1 = 2;
zersious.health1 = 10;
zersious.calculateWinner();}
System.out.println("");
System.out.println("Fight again?");
System.out.println("1 for Yes");
System.out.println("2 for No");
restart = input.nextInt();
if(restart == 1){
System.out.println("");
}else if(restart == 2){
System.out.println("Thank you for playing!");
break;}
}
}
if(hero == 4){System.out.println("You have chosen Balrock!");
for(hero = 4;;){
System.out.println("");
match = 1+battle.nextInt(5);
if(match == 1){
System.out.println("Lord Valon!");
System.out.println("");
balrock.attack1 = 6;
balrock.defense1 =0;
balrock.health1 = 19;
balrock.calculateWinner();
}else if(match == 2){
System.out.println("Prince Zersious!");
System.out.println("");
balrock.attack1 = 5;
balrock.defense1 =2;
balrock.health1 = 18;
balrock.calculateWinner();
}else if(match == 3){
System.out.println("Rintar!");
System.out.println("");
balrock.attack1 = 8;
balrock.defense1 =1;
balrock.health1 = 16;
balrock.calculateWinner();
}else if(match == 4){
System.out.println("Hawkeye!");
System.out.println("");
balrock.attack1 = 7;
balrock.defense1 =1;
balrock.health1 = 17;
balrock.calculateWinner();
}else if(match == 5){
System.out.println("Yusef!");
System.out.println("");
balrock.attack1 = 13;
balrock.defense1 =2;
balrock.health1 = 10;
balrock.calculateWinner();}
System.out.println("");
System.out.println("Fight again?");
System.out.println("1 for Yes");
System.out.println("2 for No");
restart = input.nextInt();
if(restart == 1){
System.out.println("");
}else if(restart == 2){
System.out.println("Thank you for playing!");
break;}
}
}
if(hero == 5){System.out.println("You have chosen Hawkeye!");
for(hero = 5;;){
System.out.println("");
match = 1+battle.nextInt(5);
if(match == 1){
System.out.println("Lord Valon!");
System.out.println("");
hawkeye.attack1 = 6;
hawkeye.defense1 = 0;
hawkeye.health1 = 19;
hawkeye.calculateWinner();
}else if(match == 2){
System.out.println("Prince Zersious!");
System.out.println("");
hawkeye.attack1 = 5;
hawkeye.defense1 = 2;
hawkeye.health1 = 18;
hawkeye.calculateWinner();
}else if(match == 3){
System.out.println("Balrock!");
System.out.println("");
hawkeye.attack1 = 10;
hawkeye.defense1 = 0;
hawkeye.health1= 15;
hawkeye.calculateWinner();
}else if(match == 4){
System.out.println("Rintar!");
System.out.println("");
hawkeye.attack1 = 8;
hawkeye.defense1 = 1;
hawkeye.health1 = 16;
hawkeye.calculateWinner();
}else if(match == 5){
System.out.println("Yusef!");
System.out.println("");
hawkeye.attack1 = 13;
hawkeye.defense1 = 2;
hawkeye.health1 = 10;
hawkeye.calculateWinner();}
System.out.println("");
System.out.println("Fight again?");
System.out.println("1 for Yes");
System.out.println("2 for No");
restart = input.nextInt();
if(restart == 1){
System.out.println("");
}else if(restart == 2){
System.out.println("Thank you for playing!");
break;}
}
}
if(hero == 6){System.out.println("You have chosen Yusef!");
for(hero = 6;;){
System.out.println("");
match = 1+battle.nextInt(5);
if(match == 1){
System.out.println("Lord Valon!");
System.out.println("");
yusef.attack1 = 6;
yusef.defense1 = 0;
yusef.health1 = 19;
yusef.calculateWinner();
}else if(match == 2){
System.out.println("Prince Zersious!");
System.out.println("");
yusef.attack1 = 5;
yusef.defense1 = 2;
yusef.health1 = 18;
yusef.calculateWinner();
}else if(match == 3){
System.out.println("Balrock!");
System.out.println("");
yusef.attack1 = 10;
yusef.defense1 = 0;
yusef.health1 = 15;
yusef.calculateWinner();
}else if(match == 4){
System.out.println("Rintar!");
System.out.println("");
yusef.attack1 = 8;
yusef.defense1 = 1;
yusef.health1 = 16;
yusef.calculateWinner();
}else if(match == 5){
System.out.println("Hawkeye!");
System.out.println("");
yusef.attack1 = 13;
yusef.defense1 = 2;
yusef.health1 = 10;
yusef.calculateWinner();}
System.out.println("");
System.out.println("Fight again?");
System.out.println("1 for Yes");
System.out.println("2 for No");
restart = input.nextInt();
if(restart == 1){
System.out.println("");
}else if(restart == 2){
System.out.println("Thank you for playing!");
break;}
}
}
}
}

package Tests;

import java.util.Random;

public class Duel {

Random battle = new Random();

int newHealth, newHealth1;
int outcome, outcome1, outcome2, outcome3;
int attack, attack1;
int defense, defense1;
int health, health1;

void calculateWinner(){
do{ outcome = attack - defense1;
newHealth1 = health1 - outcome;
System.out.println("Your attack does " + outcome + " damage!");
System.out.println("");
System.out.println("Enemy Health");
System.out.println(newHealth1);
System.out.println("");
outcome2 = attack1 - defense;
newHealth = health - outcome2;
System.out.println("Enemies attack does " + outcome2 + " damage!");
System.out.println("");
System.out.println(newHealth);
if(newHealth1 > 0 && newHealth > 0){
outcome = attack - defense1;
outcome1 = 2 * outcome;
newHealth1 = health1 - outcome1;
System.out.println("Your attack does " + outcome + " damage!");
System.out.println("");
System.out.println("Enemy Health");
System.out.println(newHealth1);
outcome = attack1 - defense;
outcome1 = 2 * outcome;
newHealth = health - outcome1;
System.out.println("");
System.out.println("Enemies attack does " + outcome2 + " damage!");
System.out.println("");
System.out.println(newHealth);
if(newHealth1 > 0 && newHealth > 0){
outcome = attack - defense1;
outcome1 = 3 * outcome;
newHealth1 = health1 - outcome1;
System.out.println("Your attack does " + outcome + " damage!");
System.out.println("");
System.out.println("Enemy Health");
System.out.println(newHealth1);
outcome = attack1 - defense;
outcome1 = 3 * outcome;
newHealth = health - outcome1;
System.out.println("");
System.out.println("Enemies attack does " + outcome2 + " damage!");
System.out.println("");
System.out.println(newHealth);
if(newHealth1 > 0 && newHealth > 0){
outcome = attack - defense1;
outcome1 = 4 * outcome;
newHealth1 = health1 - outcome1;
System.out.println("Your attack does " + outcome + " damage!");
System.out.println("");
System.out.println("Enemy Health");
System.out.println(newHealth1);
outcome = attack1 - defense;
outcome1 = 4 * outcome;
newHealth = health - outcome1;
System.out.println("");
System.out.println("Enemies attack does " + outcome2 + " damage!");
System.out.println("");
System.out.println(newHealth);
if(newHealth1 > 0 && newHealth > 0){
outcome = attack - defense1;
outcome1 = 5 * outcome;
newHealth1 = health1 - outcome1;
System.out.println("Your attack does " + outcome + " damage!");
System.out.println("");
System.out.println("Enemy Health");
System.out.println(newHealth1);
outcome = attack1 - defense;
outcome1 = 5 * outcome;
newHealth = health - outcome1;
System.out.println("");
System.out.println("Enemies attack does " + outcome2 + " damage!");
System.out.println("");
System.out.println(newHealth);}
}
}
}
}while(newHealth1 > 0 && newHealth > 0);
if(newHealth1 <= 0 && newHealth > 0){
System.out.println("You win!");}
if(newHealth <= 0 && newHealth1 > 0){
System.out.println("You lose!");}
if(newHealth <= 0 && newHealth1 <= 0){
System.out.println("Draw!");}
}
}

-

First, I'd like to start off by saying that if you've really only been coding in Java a few days -- well done. For "a few days'" worth of experience, having code that compiles and does what you want it to (and expect it to) is the most important thing. That being said, this is a code review site, so some suggestions....

## The combatants

As others have mentioned, my first change would be to create a class to hold data for each participant of the Duel. The basic information for each participant is their name, and integer values for their attack, defense, and health. Since naming this class 'Character' would be in conflict with java.lang.Character, I figure "Player" would be a good name:

public class Player {
private String name;
private int attack;
private int defense;
private int health;

// Contructor, getters, and setters here
}


Now we can create a Player object for each combatant:

Player valon = new Player("Valon", 6, 0, 19);


Since a "Duel" will continue until one player is "dead", I'd add a utility method for determining if a player is dead (or alive, in this case.) This will allow us to change the criteria in the future in a more simple way:

// in the Player class
public boolean isAlive() {
return health > 0;
}


Why? In object-oriented programming, a single "concept" is defined by a class, which defines its data fields and methods. Here, we are taking the "concept" of a player and putting the logic for each player into a single, well-defined object. Now the Player object for Hawkeye knows everything there is to know about Hawkeye, and it is no longer scattered throughout the code. If we want to make Hawkeye a little weaker by changing his attack value, we now only need to change it in all places instead of tracking down all the places in the code where we define him as a combatant.

## The fight

From reading the code, it looks like a "duel" is supposed to proceed by each player attacking the other until one player is dead. That is, Player 1 attacks Player 2. Then Player 2 attacks Player 1. And so on. A Player's damage is defined as their "attack" value minus their opponent's "defense" value. The damage is then subtracted from the defender's health value. The duel continues until one or both players are "dead". Since the code allows for "draws", which appears intentional, then both the attack and the counter attack (player 1 attacks player 2, player 2 attacks player 1) happen in the same round, and only after both attacks are completed does the round end and the end conditions get checked.

The duel logic can therefore be consolidated into a small method that takes only two Player variables, the duel participants. (Here I left them named "you" and "enemy" but it's really not important.)

public void duel(Player you, Player enemy) {
do {
attack(you, enemy);
attack(enemy, you);

} while (you.isAlive() && enemy.isAlive());

if (you.isAlive()) {
System.out.println("You win!");
} else if (enemy.isAlive()) {
System.out.println("You lose!");
} else {
System.out.println("Draw!");
}
}


I have extracted the "attack" code into a single method called attack. This method models the act of one Player attacking another Player, and it prints the result.

private void attack(Player attacker, Player defender) {

int damage = Math.max(0, attacker.getAttack() - defender.getDefense());
int health = defender.getHealth() - damage;

System.out.println(attacker.getName() + "'s attack does " + damage + " damage!\n");
System.out.println(defender.getName() + "'s Health\n");
System.out.println(health);

defender.setHealth(health);
}


There is one very important thing to note here, and that is damage calculation. Previously the code didn't take into account the possibility that the defender's defense value could be higher than the attacker's attack value, which would result in negative damage. Since having health go up after an attack made no sense, called the Math.max method and passed it a 0, so that if the calculated damage was negative, to default to zero.

(Another thing to note is that the concept of "you" and "your enemy" went away here and instead the attacker and defender are referred to by name. So instead of "Your attack does 20 damage!" it'll now print "Hawkeye's attack does 20 damage!")

Why? A Duel is a self-contained action between two Players, so it should be separate from the Player class itself. When we moved the logic for each combatant's stats and names into the Player object, we effectively anonymized the concept of a duel here as well. It no longer matters who "you" are and who your "enemy" is, but rather what two combatants are participating. This way we can simplify our code greatly and model an "attack" in a player-agnostic fashion.

## The arena

Now that we've defined how a duel is performed, we need to take a step back and consider this class that we just modified.

As we've defined it, a duel is an action, something that happens. It makes sense to model it as a method. If we have Valon and Hawkeye, and we want them to duel, we call duel and pass the method the parameters for those players: duel(valon, hawkeye).

The question now becomes, who is the arbiter of the combatants? That is, where is this "library" of all the available fighters defined? Furthermore, where does this duel method live?

The answer the both these questions is one and the same: the "duel" happens in an "arena". Furthermore, all the combatants gather at the "arena". So it makes sense that instead of having the class named Duel, the class is named Arena instead. Furthermore, when we construct a new Arena, we can create our gallery of available fighters as well.

I figure that since we're identifying each fighter by name, we can store our fighters in a Map object, using the name as the key. If you're new to Java you may not have heard of this data structure, but Maps are a supremely useful way of creating a set of "key-value" pairs.

Here is a class-level variable to hold the combatants, plus a constructor to populate that variable. I've also added a method to "get" a combatant by name; I will explain why after the code.

public class Arena { // formerly Duel
private Map<String, Player> combatants;

public Arena() {
// initialize the combatants
combatants = new HashMap<String, Player>();

Player valon = new Player("Valon", 6, 0, 19);
Player rintar = new Player("Rintar", 8, 1, 16);
Player zersious = new Player("Zersious", 5, 2, 18);
Player balrock = new Player("Balrock", 10, 0, 15);
Player hawkeye = new Player("Hawkeye", 7, 1, 17);
Player yusef = new Player("Yusef", 13, 2, 10);

combatants.put("Valon", valon);
combatants.put("Rintar", rintar);
combatants.put("Zersious", zersious);
combatants.put("Balrock", balrock);
combatants.put("Hawkeye", hawkeye);
combatants.put("Yusef", yusef);
}

public Player getCombatant(String name) {
Player combatant = combatants.get(name);

if (combatant == null) {
System.out.println("No combatant with name " + name + " was found!");
return null;
} else {
return new Player(combatant.getName(), combatant.getAttack(), combatant.getDefense(), combatant.getDefense());
}
}

// attack and duel methods go here
}


There are two reasons for the getCombatant method. One is not applicable entirely now, but something to keep in mind for the future. The other actually is applicable in this immediate use case.

Effectively, what getCombatant does is look for a Player with the provided name. If it can't find that Player -- eg. if the map has no key with that name -- then it prints out an error message and returns a null object. If it does find the player, it does not return that same object, but returns a copy of that object. The reasons for returning a copy are:

1. This game allows for multiple duels to happen. However, the duel and attack methods work by "winding down" the health of the character until one or both players have zero or less health. At this point, your combatants are dead. So if you decide to have a new duel, you'll somehow need to reset those combatants' health. By always pulling a "fresh" version of the combatant (by returning a copy instead of the original), the Arena will always provide you a Player with their full health.
2. If this game were multi-threaded, we'd be concerned about the safe publication of shared objects. Clearly, this is not a concern in this particular implementation, and the original post clearly says their coding level is novice, but this would be the other reason to include a copy-and-return action in this situation.

At this point, I don't have much to say about the menu and the main method. Certainly you'll want to consider Malachi's point about input validation.

But with the structure proposed here, the flow of the main method would go something like this:

1. Create a new Arena object, to instantiate the Players. Arena arena = new Arena();
2. Prompt the user for the first player's identity. Fetch that player from the arena. Player player1 = arena.getCombatant("Rintar");
3. Prompt the user for the second player's identity. Fetch that player from the arena. Player player2 = arena.getCombatant("Yusef");
4. Fight! arena.duel(player1, player2);
5. Ask the user if they want to fight again. If they do, loop back to step #2. Otherwise, exit.
-
[belated] Welcome to CR! That's an awesome first post, stick around, we can't have enough answers like this! –  Mat's Mug Jan 25 '14 at 19:48

Generally, if you have multiple lines repeating a lot with minor or no changes, you want to make them into a function or a class. Especially, if you plan on extending them later.

Also, you'd want to design your classes more carefully. It would make a lot more sense to create a Character class, that has a name, attack, health and defense. Then since a duel is a fight between two characters, you could just have two Character variables inside of it, instead of two integers for health, attack etc.

A class is essentially a combination of variables (data) and functions that work with these variables. You can save a lot of lines by creating functions for all your printing.

For instance if you make a Character class, you could fill it with data using either a function or a constructor - http://docs.oracle.com/javase/tutorial/java/javaOO/constructors.html - that would enable you to do all the setting up in a single line:

public Character(int attack, int defense, int health)
{
this.attack = attack;
this.defense = defense;
this.health = health;
}


Called as:

Character valon = new Character(6, 0, 19);


To print a character's stats, you could then make another function inside the Character class:

public void printStats()
{
System.out.println("Attack    " + this.attack);
// other stats
}


And just call it whenever you want to show the stats:

valon.printStats();


Same concepts can be applied inside the Duel class: you could have a Round class that represents A attacking B and B attacking A back. Or even an Attack class that would further split this round to two attacks.

-
Good, except that Character would be an unfortunate name for a class, as it clashes with java.lang.Character. –  200_success Dec 26 '13 at 12:06

there are a couple of things that I noticed, and I don't think that anyone else touched on these, but they are very important for a beginner in any language.

1. indentation

• this includes ending brackets. make sure that your ending brackets are like this:

            }
}
}
}


And not like this

}
}
}
}

• your code should look like this

public class ClassName {
int variable1;
int variable2;
int variable3;

void methodName() {

/* Do Stuff */
}
}

• everything inside of if statements should also be indented.
2. do while loops. lot's of people don't like them, you should turn the do while loop inside of calculateWinner() into a while(newHealth1 > 0 && newHealth > 0)

upon reaching this point I have realized that this code doesn't work.

all of your NPC and Player objects are type Duel and Duel is where the calculateWinner method is located, but it doesn't take any input parameters that would suggest that it is expected to interact with another Duel object and

What you need is more classes,

1. player class, made up of

• Duel object (you should probably rename)
• might want to include a challenge method as well
2. Enemy class (you may not need this depending on how you set up your player class)

3. Duel class needs to be changed significantly

I haven't put much time into this, because there is a lot of if then nonsense going on in this code that can be cleaned up.

I am not sure why you are doing this either

 if(hero == 5){System.out.println("You have chosen Hawkeye!");
for(hero = 5;;){ ...


the way that the rest of the code is written it should be while statement written like this

int restart = 1; //for clarity
if(hero == 5)
{
System.out.println("You have chosen Hawkeye!");

while (restart == 1)
{
/* rest of code */
System.out.println("Fight again?");
System.out.println("1 for Yes");
System.out.println("2 for No");
restart = input.nextInt();
}
}


and you should be validating user input as well, what if they input 3?

This answer seems really scattered to me, but it's early in the morning.

-

Yes it could be much smaller and therefore more easy to read.

For example, you quite often set the fields: attack, defense, health. This could be moved to a method which sets them all three at once.

  public setValues(int attack, int defense, int health){
this.attack = attack;
this.defense = defense;
this.health = health;
}


Also you could define a constructor which takes these three values.

/**Constructor which initializes: attack, defense, health*/
public Duel(int attack, int defense, int health){
this.setValues(attack, defense, health);
}


You can also improve the block System.out.println("Choose your HERO"); ... by extracting a method, which prints the values of a hero.

Not shorter but better style: use switch(match) instead of
if(match== ..) ... else if(match == ..) ...

And there is quite a lot of other code which looks like copy and paste and should be extracted to methods.

-
+1 for switch. –  syb0rg Dec 26 '13 at 18:15