Two ways of implementing a combat system

Question

Below is a segment of a game I am writing to stay in practice with my Java.

EDIT The class this method is in is the Encounter class. This class just manages the encounter between the player and an enemy, and the other methods are victory() and defeat(), which add gold/experience when player wins, and removes gold when player loses.

There are two ways I am thinking of implementing a combat system, but I am not sure which path is more efficient. The code works perfectly fine; I am only inquiring about the efficiency:

EDIT 2 Included the entire class

Option 1

import java.util.Scanner;
public class Encounter {
 private static Scanner sc;
 private static Player player;
 private static Enemy enemy;
 public static void manage(Player p, Enemy e) {
 player = p;
 enemy = e;
 sc = new Scanner(System.in);
 System.out.println("You are fighting a " + enemy.getName() + "!");
 while(player.getHealth() > 0 && enemy.getHealth() > 0) {
 System.out.println(player.getName() + "'s Health/Mana: " + player.getHealth() + "/" + player.getMana());
 System.out.println(enemy.getName() + "'s' Health/Mana: " + enemy.getHealth() + "/" + enemy.getMana());
 Ability playerAbility = selectAbility();
 //Player attacks enemy => Enemy loses health => player loses mana
 enemy.subtractHealth(playerAbility.getDamage());
 player.subtractMana(playerAbility.getManaCost());
 //Enemy attacks player => Player loses health
 player.subtractHealth(enemy.attackPlayer());
 }
 if(player.getHealth() <= 0) {
 defeat();
 } else if(enemy.getHealth() <= 0) {
 victory();
 }
 }
 private static void defeat() {
 System.out.println("=======================");
 System.out.println("You died to " + enemy.getName() + "!");
 System.out.println("=======================");
 //Handle death here
 player.deathSubtraction();
 player.resetHealthMana();
 }
 private static void victory() {
 System.out.println("You defeated " + enemy.getName() + "!");
 //Handle experience/gold/item(s) here
 player.addExperience(enemy.getExperience());
 player.addGold(enemy.getGold());
 player.checkLevelUp(player);
 displayStats();
 player.resetHealthMana();
 System.out.println("Press enter to continue");
 sc.nextLine();
 }
 private static Ability selectAbility() {
 player.displayMoves();
 while(true) {
 String input = sc.nextLine();
 for(Ability ability : player.getAbilities()) {
 if(input.equalsIgnoreCase(ability.getName()) && ability.getManaCost() < player.getMana()) {
 return ability;
 }
 }
 System.out.println("Not a valid move/Not enough mana! Select again!");
 }
 }
 private static void displayStats() {
 System.out.println("============================");
 System.out.println("Current Level: " + player.getLevel());
 System.out.println("Current Experience: " + player.getExperience() + "/100");
 System.out.println("Current Gold: " + player.getGold());
 System.out.println("============================");
 }
}

Option 2

import java.util.Scanner;
public class Encounter {
 private static Scanner sc;
 private static Player player;
 private static Enemy enemy;
 public static void manage(Player p, Enemy e) {
 player = p;
 enemy = e;
 sc = new Scanner(System.in);
 System.out.println("You are fighting a " + enemy.getName() + "!");
 while(true) {
 System.out.println(player.getName() + "'s Health/Mana: " + player.getHealth() + "/" + player.getMana());
 System.out.println(enemy.getName() + "'s' Health/Mana: " + enemy.getHealth() + "/" + enemy.getMana());
 Ability playerAbility = selectAbility();
 //Player attacks enemy => Enemy loses health => player loses mana
 enemy.subtractHealth(playerAbility.getDamage());
 player.subtractMana(playerAbility.getManaCost());
 //Enemy attacks player => Player loses health
 player.subtractHealth(enemy.attackPlayer());
 if(player.getHealth() <= 0) {
 defeat();
 break;
 } else if(enemy.getHealth() <= 0) {
 victory();
 break;
 }
 }
 }
 private static void defeat() {
 System.out.println("=======================");
 System.out.println("You died to " + enemy.getName() + "!");
 System.out.println("=======================");
 //Handle death here
 player.deathSubtraction();
 player.resetHealthMana();
 }
 private static void victory() {
 System.out.println("You defeated " + enemy.getName() + "!");
 //Handle experience/gold/item(s) here
 player.addExperience(enemy.getExperience());
 player.addGold(enemy.getGold());
 player.checkLevelUp(player);
 displayStats();
 player.resetHealthMana();
 System.out.println("Press enter to continue");
 sc.nextLine();
 }
 private static Ability selectAbility() {
 player.displayMoves();
 while(true) {
 String input = sc.nextLine();
 for(Ability ability : player.getAbilities()) {
 if(input.equalsIgnoreCase(ability.getName()) && ability.getManaCost() < player.getMana()) {
 return ability;
 }
 }
 System.out.println("Not a valid move/Not enough mana! Select again!");
 }
 }
 private static void displayStats() {
 System.out.println("============================");
 System.out.println("Current Level: " + player.getLevel());
 System.out.println("Current Experience: " + player.getExperience() + "/100");
 System.out.println("Current Gold: " + player.getGold());
 System.out.println("============================");
 }
}

I am unsure if I should just break when the conditions are met, or have the condition be evaluated in the while loop. I feel that having while(true) can avoid any bugs, and a simple break will ensure that it exits the loop. Any and all suggestions are appreciated and considered.

• 200_success: Please provide some more context to this code. What class does it appear in, and what else does the class do? (The player = p stuff at the top of the function looks suspiciously bad.)
• Ben A: @200_success Added more clarification
• 200_success: I would prefer that you actually show the code. I believe that the design of the class is fundamentally flawed. As @Martin's answer says, static shouldn't be used.
• Ben A: @200_success Included entire class

Answer 1

• You've used a readable idiomatic Java naming convention.
• There is no point reassign p to player and e to enemy, use the full name on entry, make the parameters final.
• Static methods are a code smell in OO programming, they will result is tightly coupled code. They should be studiously avoided until you know the specific circumstances that make them necessary.
• Follow the tell don't ask idiom , tell the player to attack the enemy and tell the enemy to attack the player which will minimise coupling if you use an interface.
• Never use while(true). In this case I would use a do { .. } while(...) loop since you always want this loop to be performed once. Use while(...) when it could be 0..N and do { .. } while(...) when the loop is 1..N.
  • Use a single condition, probably the player's input or battling continues for your loop control for clarity.
• Put the health display inside the Player and Enemy classes (or a common base class) and show it during players and enemy turn.
• Pass the gold & experience as a parameter to the victory method on the player.

Something like this:

public void manage(Player player, Enemy enemy) {
 boolean battling = true; 
 do {
 player.turn(enemy);
 enemy.turn(player);
 battling = player.isAlive() && enemy.isAlive();
 } while(battling);
 }
}

• In the Player and Enemy classes have a Opponent interface (follow the Interface Segregation Principle) that includes a turn method to makes attack and takes damage. The instances of Player and Enemy sends damage to the opponent and reduces their karma. Getting and Setting is a code smell that increases coupling.

e.g.

public turn(Opponent character) { 
 displayStatus();
 attack = chooseAttack();
 reduceMana(attack);
 character.damage(attack);
}
public damage(Attack attack) { 
 hp - hp - attack.damage;
}

• 200_success: There should be no need for your battling flag variable, if you structure the loop properly.
• Martin Spamer: I regard clarity as a first class nfr of my team members.