Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 bliz 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;     
                }
            }
        }
    }
}
share|improve this question
4  
"Please enter the a" + "mount [...] separa" + "ted by a space."?! I thought splitting the string on multiple lines was supposed to improve readability! –  Mat's Mug Dec 4 '13 at 15:29
    
@retailcoder - while funny, not constructive. –  Max Dec 4 '13 at 15:30
    
@Max sorry I didn't mean to offend anyone - it's just one of the point's I'd raise if I were to review this code. –  Mat's Mug Dec 4 '13 at 15:34
2  
I'd upvote, but I'm out of ammo... will come back at 12AM UTC for sure :) –  Mat's Mug Dec 4 '13 at 15:37
4  
I took the liberty to roll the edit back, please do not change the code after posting it. This would invalidate existing answers and is unnecessary. If it was a hiccup while posting, you can alwazs comment and tell people that. –  Bobby Dec 4 '13 at 17:32
show 3 more comments

4 Answers 4

up vote 12 down vote accepted

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

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)
share|improve this answer
    
Wow, just wow. This is just, amazing! It's going to take me a few hours with Google to understand some of the things you have done but it should be well worth it! –  lukeb28 Dec 5 '13 at 2:18
    
One question: how does the before.attack and after.attack work? I can never see where "after" comes in except at the end. –  lukeb28 Dec 5 '13 at 3:01
    
In main()Strength after = roller.play(before). That means that before represents the pre-battle state, and after represents the post-battle state. –  200_success Dec 5 '13 at 3:42
    
Are those lines of slashes really necessary? –  Bobby Dec 5 '13 at 8:21
    
@Bobby I just find it very helpful to demarcate inner classes clearly. –  200_success Dec 5 '13 at 8:23
show 1 more comment
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.

share|improve this answer
    
@ variable name "k" I forgot to rename that to something better didn't I? I'll get that sorted! bltz should be a better name. 1/? –  lukeb28 Dec 4 '13 at 16:29
6  
Great review... one (really) small nitpick ... if(!"".equals(Lsr)) ... is not the same as !Lsr.equals("")... Sometimes people (including me) use "constant".equals(val) to avoid possible NullPointerExceptions with the alternative. Otherwise, great! –  rolfl Dec 4 '13 at 16:29
    
@ Lsr, sounds good. I'll fix that. @ bad strings, I did fix that, you must have started that answer before I did. 2/? –  lukeb28 Dec 4 '13 at 16:32
2  
@lukeb28: I'll not go into details about Object Oriented programming and abstraction, that's what good books are for and it it's out of scope for Code Review. It's basically about how your program has no real structure compared to what Java allows you to do. I've also extended my answer with some links which should answer most of your questions. Otherwise no need to apologize, as along as you're willing to learn it's okay to make mistakes and ask questions. –  Bobby Dec 4 '13 at 17:41
1  
@lukeb28: Also good names are not shortened. It's okay if your variables are named counterForAllFailedAttempts if it describes what the variable is for. Don't be afraid of long variable names. Always ask yourself "would somebody else understand what I mean with this?", good code is readable and clearly structured, it eases working with it and allows for easier changing it later on. Always go for readability first. –  Bobby Dec 4 '13 at 17:45
show 6 more comments

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/62 (6 (62 - 52) + 5 (52 - 42) + 4 (42 - 32) + 3 (32 - 22) + 2 (22 - 12) + 1) ≈ 4.4722

The expected value of the maximum of three dice is

1/63 (6 (63 - 53) + 5 (53 - 43) + 4 (43 - 33) + 3 (33 - 23) + 2 (23 - 13) + 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.)

share|improve this answer
    
Thanks for pointing those rule errors out, no one else noticed and I didn't either. I used to have it work properly but I guess when I re-coded some of it (The code I show here is the result of about a week playing around with it) I got rid of that functionality without knowing. As for the Blitz mode, that's more of a balancing factor that I think would work well in the real world as if your committing to an all or nothing attack you should have an ace up your sleeve. I know a 7 sided die does not exist, it just that anything else that does would be too great a multiplier for it to be fair. –  lukeb28 Dec 5 '13 at 2:27
    
Fixed that by adding "&& i < atck1" in the for loop's conditional. –  lukeb28 Dec 5 '13 at 2:45
add comment

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.

share|improve this answer
    
Minor nitpick: Please spell all words with vowels, there's no point in shortening variable names at all (the compiler does not care, the JVM does not care, your harddisk most likely also not...who will care is the next coder who needs to change something in that code). –  Bobby Dec 4 '13 at 17:49
    
@Bobby - I removed "short" and "variable" so it applies to all words. Is this what you meant? –  David Harkness Dec 4 '13 at 18:19
    
Kinda, I meant that you should never omit vowels from variable names, no matter if short or long. –  Bobby Dec 4 '13 at 19:10
    
@Bobby "no point in shortening variable names at all (the compiler does not care" Didn't know that. I thought it did help. I will keep this in mind for future projects –  lukeb28 Dec 4 '13 at 22:47
    
@lukeb28 With high CPU speeds and massive amounts of memory, you don't have to worry about optimizing for the compiler on today's PCs--most especially while learning to program. Developer time is far more valuable than hardware. –  David Harkness Dec 5 '13 at 22:49
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.