Risk battle simulator v2

I wrote a Risk battle simulator that returned the odds of every possible outcome and put it up to be reviewed here. I used the comments to improve it, and am wondering if it can be optimized further.

```
#include
#include
#include
#include

using namespace std;

class battle {
public:
int attackers;
int defenders;
float odds;
battle (int a, int d, float o) {
attackers = a;
defenders = d;
odds = o;
}
};

void submain(int, int);
void cleaner(vector&);
void sorter(battle);
void a1d1(battle);
void a1d2(battle);
void a2d1(battle);
void a2d2(battle);
void a3d1(battle);
void a3d2(battle);
void giveoutput();
int oddssize();

vector attack_wins, defend_wins;
vector outcomes;

int main() {
int attackers, defenders;
cout > attackers;
cout > defenders;

submain(attackers, defenders);

giveoutput();
}

// The main part of the program
// Has the list of possibilities condensed, then has each possibility ran
void submain(int attackers, int defenders){
attack_wins = vector(attackers,0);
defend_wins = vector(defenders,0);
outcomes.push_back(battle(attackers, defenders, 100));

while (!outcomes.empty()){
vector list = outcomes;
outcomes.clear();
cleaner(list);

for (int l = 0; l & list){
static int count;
static bool fastshrink = 1;
count -= 1 + fastshrink;

if(count d1)
ATK++;
else
DEF++;
}
}
}

outcomes.push_back(battle(fight.attackers, fight.defenders - 1, fight.odds * ATK /SUM));
outcomes.push_back(battle(fight.attackers - 1, fight.defenders, fight.odds * DEF / SUM));
}

void a1d2(battle fight) {
static int ATK = 0;
static int DEF = 0;
static int SUM = 0;

if(!SUM) {
for(int a1=1;a1D1)
ATK++;
else
DEF++;
}

Don't use using namespace std. (Yes, you said don't bring it up. Doesn't mean I have to listen to you. Similar principles tend to apply in the real world.)

Your indentation is a bit screwy; it actually took me longer than I care to admit to figure out that there wasn't a word or two blanked out in

float odds;
           battle (int a, int d, float o) {

Also, at least for the free (non-inline-member-function) functions, I'd strongly advise putting a newline before the opening brace of the function. Almost every C++ codebase out there uses some variation on

void f()
{
    ...
}

and not

void f() {
    ...
}

There are exceptions, of course, but they're outnumbered by the non-exceptions.

Your use of global variables (lots of them) is a problem. Try to move them into the scopes of the functions that use them, instead of exposing them to the entire program.

Again I spent a few seconds staring at a line of code wondering if something was missing:

attack_wins = vector(attackers,0);

This line certainly looks like it's missing an auto at the beginning! C++ isn't Python, after all. But it turns out that attack_wins is a global variable, and so this line is correct — it's merely an expensive and confusing way of writing

attack_wins.clear();

This is a code smell:

void a1d1(battle);
void a1d2(battle);
void a2d1(battle);
void a2d2(battle);
void a3d1(battle);
void a3d2(battle);

I have no specific suggestions for how to clean it up (I don't know Risk well enough, and haven't really thought about how to enumerate all the die rolls other than "run a naive simulation for each s in 1..6large number, using a random number generator that just returns the base-6 digits of s"), but it certainly smells like it could be written in about one-sixth the code you've got.

Actually, you yourself point out that each of these six functions could be a two-or-three-liner, but you wrote out all the nested loops and switches to make them "easy to change". Even so, I bet you could come up with a way to abstract out the idea of "roll M dice, roll N dice, compare them pairwise, summarize the result" that involves less code duplication than this.

void cleaner(vector& list)

Please don't use the word list to describe a std::vector. Use v, or vec, or something, but please not list. The only time you should use the word list in a C++ program is when you're referring to an actual std::list. (Which should basically never happen.)

The cleaner function has a couple of parts that I would characterize as... problematic. It's got a static int count, which looks like it wants to be a function parameter; and then it's got this static int fastshrink, which I'm not sure what that is. Turn those into function parameters, and/or get rid of them.

It seems like the non-problematic parts of this function could be replaced with just a couple lines of C++ code involving std::partition and std::accumulate (or a plain old for-loop with a += inside it, if you prefer). You definitely shouldn't need to be repeatedly eraseing from the middle of the vector; that's one of the slowest things you can do, and it's 100% unnecessary, if you start by partitioning the vector so that all the items you're eventually going to erase wind up at the end of the vector.

StackExchange Code Review Q#118568, answer score: 3