patternjavaModerate

Possible improvements to Risk board game?

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

riskpossiblegameimprovementsboard

Problem

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 0 && rollCnt 1)
{
if(y[rollCnt] >= z[rollCnt])
{
atck2--;
}
else
{
dfnc2--;
}
rollCnt++;

Solution

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 = 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 `

Code Snippets

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;
        }
    }
}

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);
}

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;
}

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();
}

Context

StackExchange Code Review Q#36643, answer score: 16

Revisions (0)

No revisions yet.