Possible improvements to Risk board game?

Question

Enter risk!

This program will roll dice for the game of Risk. The initial input is two numbers separated by a space and if the attackers wish to do a blitz they can add one more space and an "!" to get a positive attack modifier but are committed for an all or nothing fight.

package riskdieroll;
import java.awt.Component;
import java.util.Arrays;
import java.util.Collections;
import javax.swing.JOptionPane;
public class RiskDieRoll 
{
    private static Component frame;
    public static void main(String[] args) 
    {
        int spcCnt, atck1, dfnc1, rollCnt, atck2 = 0, dfnc2 = 0, valHold, atckMod;
        String intInpt, Lsr;
        while(true)
        {
            boolean k = false;
            Lsr = "";
            intInpt = JOptionPane.showInputDialog("Please enter the a"
                    + "mount of attackers then the number of\ndefenders separa"
                    + "ted by a space.", atck2 + " " + dfnc2);
            spcCnt = rollCnt = 0;
            valHold = intInpt.length();
            atckMod = 6;
            if("0 0".equals(intInpt))
            {
                break;
            }
            for(int i = 0; intInpt.charAt(i) != ' '; i++)
            {
                spcCnt++;
            }
            atck2 = atck1 = (int)Double.parseDouble(intInpt.substring(0 , spcCnt));
            if(intInpt.charAt(valHold - 1) == '!')
            {
                k = true;
                atckMod = 7;
                valHold -= 2;
            }
            dfnc2 = dfnc1 = (int)Double.parseDouble(intInpt.substring(spcCnt + 1 , valHold));
            Integer[] z = new Integer[3], y = new Integer[2];
            if(atck1 < 2)
            {
                JOptionPane.showMessageDialog(frame, "INVALID ATTACK! You need"
                        + " at least two armys to make an attack!");
                atck2 = dfnc2 = 0;
            }
            else if(dfnc1 < 1)
            {
                JOptionPane.showMessageDialog(frame, "INVALID INPUT! There must"
                        + " be at least one defending army!");
                atck2 = dfnc2 = 0;
            }
            else
            {
                do
                {
                    rollCnt = 0;
                    for(int i = 0; i < 3; i++)
                    {
                        z[i] = (int)Math.ceil(Math.random() * atckMod);
                    }
                    for(int i = 0; i < 2; i++)
                    {
                        y[i] = (int)Math.ceil(Math.random() * 6);
                    }
                    Arrays.sort(z, Collections.reverseOrder());
                    Arrays.sort(y, Collections.reverseOrder());
                    while(dfnc2 > 0 && rollCnt < 2 && atck2 > 1)
                    {
                        if(y[rollCnt] >= z[rollCnt])
                        {
                            atck2--;
                        }
                        else
                        {
                            dfnc2--;
                        }
                        rollCnt++;
                    }
                }while(k && (dfnc2 > 0 && atck2 > 1));
                if(dfnc2 < 1)
                {
                    Lsr = "\n\nDefenders have lost!!";
                }
                else if(atck2 < 2)
                {    
                    Lsr = "\n\nAttackers have been repelled!!!";
                }
                JOptionPane.showMessageDialog(frame, "Attacking force now at " 
                        + atck2 + " (Lost " + (atck1 - atck2) + ")" + "\nDefence force now "
                        + "at " + dfnc2 + " (Lost " + (dfnc1 - dfnc2) + ")" + Lsr);
                if(!"".equals(Lsr))
                {
                    atck2 = dfnc2 = 0;     
                }
            }
        }
    }
}

Answer 1

This code turned out to be a great exercise in refactoring, primarily using

• Extract Method
• Reduce Scope of Variable
• Extract Class
• Introduce Explaining Variable
• Self Encapsulate Field

Here's my goal:

public class RiskDieRoller
{
    //////////////////////////////////////////////////////////////////////

    private static class Strength
    {
        public int attack;
        public int defence;
        public int attackMod = 6;

        public Strength() {}

        public Strength(Strength other)
        {
            this.attack = other.attack;
            this.defence = other.defence;
            this.attackMod = other.attackMod;
        }

        public void blitz()
        {
            this.attackMod = 7;
        }

        public boolean isBlitz()
        {
            return this.attackMod == 7;
        }

        public String toString()
        {
            return "Attack " + this.attack +
                   " Defence " + this.defence +
                   " AttackMod " + this.attackMod;
        }
    }

    //////////////////////////////////////////////////////////////////////

    // …

    public static void main(String[] args) 
    {
        RiskDieRoller roller = new RiskDieRoller();

        Strength before = new Strength();
        while (null != (before = roller.prompt(before)))
        {
            Strength after = roller.play(before);
            boolean isDecisive = roller.report(before, after);
            before = isDecisive ? new Strength() : after;
        }
    }
}

I've renamed the class to RiskDieRoller and introduced a Strength class to help tame the proliferation of variables. The main() function just outlines how the program flows.

It's now just a simple matter of filling in the blanks. ☺︎

---

Prompt

You failed to handle the "Cancel" button — a NullPointerException occurs. You handle "0 0" as a special case, but what if there is gratuitous extra whitespace? Also, if the exclamation mark is not preceded by a space, then Double.parseDouble() barfs. Anyway, you should be calling Integer.parseInt() instead.

In any case, analyzing the string character by character is tedious, error prone, and unforgiving of variations in the user-provided input. Just use a regular expression.

private static final Pattern INPUT_RE =
    Pattern.compile("^\\s*(\\d+)\\s+(\\d+)\\s*(!)?\\s*$");

public Strength prompt(Strength before)
{
    do
    {
        String input = JOptionPane.showInputDialog("Please enter the "
                + "number of attackers then the number of\ndefenders "
                + "separated by a space.",
                before.attack + " " + before.defence);
        if (null == input)
        {
            return null;
        }

        Matcher matcher = INPUT_RE.matcher(input);
        if (matcher.matches())
        {
            Strength s = new Strength();
            s.attack = Integer.parseInt(matcher.group(1));
            s.defence = Integer.parseInt(matcher.group(2));
            if (null != matcher.group(3))
            {
                s.blitz();
            }

            if (s.attack == 0 && s.defence == 0)
            {
                return null;
            }
            if (s.attack <= 1)
            {
                output("INVALID ATTACK! You need"
                        + " at least two armies to make an attack!");
            }
            else if (s.defence <= 0)
            {
                output("INVALID INPUT! There must"
                        + " be at least one defending army!");
            }
            else
            {
                return s;
            }
        }
    } while (true); // Repeat until input passes validation
}

private void output(String message)
{
    JOptionPane.showMessageDialog(null, message);
}

It's not necessary to have a null Component instance variable; you can just pass a literal null to .showMessageDialog().

---

Play

This part of the code is more or less the same as before, but with nicer variable names. I've changed (int)Math.ceil(Math.random() * …) to Random.nextInt().

private Random random = new Random();

public Strength play(Strength before)
{
    Strength s = new Strength(before); // Return a copy
    Integer[] attackRolls = new Integer[3],
              defenceRolls = new Integer[2];

    do
    {
        for (int i = 0; i < 3; i++)
        {
            attackRolls[i] = 1 + random.nextInt(s.attackMod);
        }
        for (int i = 0; i < 2; i++)
        {
            defenceRolls[i] = 1 + random.nextInt(6);
        }
        Arrays.sort(attackRolls, Collections.reverseOrder());
        Arrays.sort(defenceRolls, Collections.reverseOrder());

        for (int rollCnt = 0; rollCnt < 2; rollCnt++)
        {
            if (s.defence <= 0 || s.attack <= 1)
            {
                return s;
            }
            if (defenceRolls[rollCnt] >= attackRolls[rollCnt])
            {
                s.attack--;
            }
            else
            {
                s.defence--;
            }
        }
    } while (s.isBlitz())
    return s;
}

Strictly speaking, you don't have to add 1 to .nextInt(), but it's more human-friendly to do so.

---

Report

Again, the main difference is in the management of variables. This function now has to return true if the battle was decisive. Arguably, that violates the single-responsibility principle, and could be improved further.

In your original code, you checked for dfnc2 > 0 to continue the battle and dfnc2 < 1 to report annihilation of the defenders. I've changed this to s.defence <= 0 to check for termination of the battle and after.defence <= 0 to report annihilation, so that the similarity is apparent.

I think String.format() is more readable than concatenation.

public boolean report(Strength before, Strength after)
{
    String verdict = (after.defence <= 0) ?
                        "\n\nDefenders have lost!!" :
                     (after.attack <= 1)  ?
                        "\n\nAttackers have been repelled!!!" : "";

    output(String.format("Attacking force now at %d (Lost %d)\n" +
                         "Defence force now at %d (Lost %d)" +
                         "%s",
                         after.attack,
                         before.attack - after.attack,
                         after.defence,
                         before.defence - after.defence,
                         verdict));

    return !verdict.isEmpty();
}

---

Nitpicks

• "armys" → "armies"
• "a" + "mount of attackers" → "number of attackers" (because it's a countable noun)

Answer 2

package riskdieroll;

Package names are used to associate developers/companies with the code. They normally look like this:

package com.company.yourpackage;
package com.gmail.username.yourpackage;

This is not a problem if this is only a test of some sorts, the moment you release code into the wild you should assign a correct package to it.

---

public static void main(String[] args) 
    {

Java uses a modified K&R style for braces, keeping opening braces on the same line:

function name() {
    if (variable) {
        // Code
    } else {
        // No Code
    }
}

---

int spcCnt, atck1, dfnc1, rollCnt, atck2 = 0, dfnc2 = 0, valHold, atckMod;

Avoid initializing multiple variables in the same line, it just gets messy and unreadable. If you initialize many variables in the same line, it can be that someone who quickly reads your code misses these variables, which leads to confusion. If you have so many variables that even initializing them line by line seems messy, there's something wrong with your code and it should most likely be restructured.

---

boolean k = false;

Here is one of the ultimate rules for programming:

Use correct, accurate and easy to remember names for all your stuff.

k is not a good name, the name should tell me what the variable is supposed to do or what it is used for. Quick, can you tell me what these variables are used for?

• k
• u
• isRunning
• nothingFound
• counter
• mmmmm
• __mmmmm

---

Lsr = "";

This variable is declared outside of the loop, yet it is cleared each iteration, therefor it's scope is limited to the loop, therefor it should be declared inside the loop to make it obvious that this variable is not persistent over iterations.

---

intInpt = JOptionPane.showInputDialog("Please enter the a"
    + "mount of attackers then the number of\ndefenders separa"
    + "ted by a space.", atck2 + " " + dfnc2);

Even if violating the 80/120 column rule, format your strings in a way that keeps them readable.

intInpt = JOptionPane.showInputDialog("Please enter the amount of attackers then the number of\n"
    + "defenders separated by a space.", atck2 + " " + dfnc2);

---

spcCnt = rollCnt = 0;

Try to avoid multiple assignments on one line unless it is kinda necessary, for the same reasons as above. Also, names.

---

atck2 = atck1 = (int)Double.parseDouble(intInpt.substring(0 , spcCnt));

Why are you parsing a Double if you want an int?

---

if(atck1 < 2)

Try to avoid magic numbers and strings at all costs! Use constants and enums to give them names. Compare these two snippets:

if (e < 5) {

// Somewhere in the class
private static final int MIN_VALUE = 5;

// Down in the function
if (value < MIN_VALUE) {

---

JOptionPane.showMessageDialog(frame, "Attacking force now at " 
    + atck2 + " (Lost " + (atck1 - atck2) + ")" + "\nDefence force now "
    + "at " + dfnc2 + " (Lost " + (dfnc1 - dfnc2) + ")" + Lsr);

Consider if String.format() might be a better options for formatting strings with many variables.

---

if(!"".equals(Lsr))

It's called "Yoda conditions". It comes up from the dark days of C/C++ when you could accidentally do this:

if (x = 5) {

Today, all compilers warn you about such things and in statically typed languages (like Java) it even does not compile at all. The difference between these two

• if(!"".equals(Lsr))
• if(!lsr.equals(""))

is a mere aesthetic one, but it's the difference between reading these two

• If an empty string is not equal to lsr
• If lsr is not equal to an empty string

Not to mention that you should test strings for emptiness by testing their length, it's faster and easier to read:

if (Lsr.length() >= 0) {

Or even better:

if (!Lsr.isEmpty()) {

This does not cover the possibility that the string is null, which will yield a NullPointerException when trying to invoke instance methods. If it is possible that the string is null, an additional check would be required with this method:

if (Lsr != null && !Lsr.isEmpty()) {

If that is acceptable depends on many things.

---

When it comes to Object Orientation, your program is not at all Object Oriented. It would benefit greatly from abstracting the logic for calculating the dice rolls into a separate class. Same goes for input and output. Consider the following

Your main loop should not contain how your program works, it should tell you what it does.

Start by separating this main-loop into separate functions. See what can stand on it's own and what you need to do to make it stand on it's own. Hint: The moment you want to use public static/global variables variables to connect these functions, you're doing something wrong.

Answer 3

Besides the stylistic issues, I think you may have made two fundamental mistakes in implementing the rules of Risk.

Number of dice rolled

According to Hasbro's rules…

• You, the attacker, will roll 1, 2 or 3 red dice: You must have at least one more army in your territory than the number of dice you roll. Hint: The more dice you roll, the greater your odds of winning. Yet the more dice you roll, the more armies you may lose, or be required to move into a captured territory.
• The defender will roll either 1 or 2 white dice: To roll 2 dice, he or she must have at least 2 armies on the territory under attack. Hint: The more dice the defender rolls, the greater his or her odds of winning-but the more armies he or she may lose.

The main issue is that in your implementation, the probabilities don't reflect the army strengths. In your code, the attacker rolls three virtual dice:

for(int i = 0; i < 3; i++)
{
    z[i] = (int)Math.ceil(Math.random() * atckMod);
}

… and similarly, the defender rolls two virtual dice. Then you sort them in descending order and decide the fate of each unit based on the top two dice of each side, stopping early if either side is annihilated. According to the rules, though, each side should only roll as many dice as the number of units involved in the battle.

The game is designed to mimic the effect of units reinforcing each other. The probabilities are not nearly the same! The expected value of one die throw is

1/6 (6 + 5 + 4 + 3 + 2 + 1) = 3.5

The expected value of the maximum of two dice is

1/6² (6 (6² - 5²) + 5 (5² - 4²) + 4 (4² - 3²) + 3 (3² - 2²) + 2 (2² - 1²) + 1) ≈ 4.4722

The expected value of the maximum of three dice is

1/6³ (6 (6³ - 5³) + 5 (5³ - 4³) + 4 (4³ - 3³) + 3 (3³ - 2³) + 2 (2³ - 1³) + 1) ≈ 4.95833

The minor issue is that you haven't implemented an option to battle with only some of the available units, which would involve more complex prompting.

Blitz

The attacker's die rolls are simulated using

(int)Math.ceil(Math.random() * atckMod)

where atckMod is 7 instead of 6 in blitz mode. That would be a simulation of a seven-sided die, which shouldn't physically exist. Maybe you meant to add 1 bonus point to each die roll instead? (Blitz mode isn't mentioned in the rules I referenced above; perhaps it's a house rule you have.)

Answer 4

To reiterate Bobby's point about using meaningful names, please consider spelling words with all their vowels in tact. This has several benefits:

• The code is much easier to read.
• You improve your prose typing skills.
• You avoid building muscle memory of misspelled words.

And seriously, you're not saving yourself any time by omitting vowels at random. In fact, it probably takes longer as you have to keep correcting yourself when you type iAttack.