5
\$\begingroup\$

I am currently teaching myself C++ in my spare time. I created the below "game" to simulate combat between an army of humans and an army of skeletons. Is there a simpler or better way of creating this game? I would love for someone to "improve" my code so I can get a better idea of how to use C++.

I think a major improvement would be to allow damage to both armies during the combat. SO you could potentially loose skeletons or humans even during their own individual turns.

ORIGINAL CODE -

#include <iostream>
#include <stdlib.h>
#include <time.h>

int main() {
    int HUMANS, SKELETONS, iSecret1, iSecret2, humanAttackPower, skeletonAttackPower;

    std::cout << "Number of human combatants:";
    std::cin >> HUMANS;
    std::cout << "Number of skeletons:";
    std::cin >> SKELETONS;

    /* initialize random seed: */
    srand (time(NULL));

    while( HUMANS > 0 && SKELETONS > 0) {
        /* generate secret number between 1 and 20: */
        iSecret1 = rand() % 20 + 1;
        humanAttackPower = rand() % HUMANS + 1;
        iSecret2 = rand() % 20 + 1;
        skeletonAttackPower = rand() % SKELETONS + 1;
        if( iSecret1 > iSecret2 ) {
            std::cout << "Humans attack with gusto!" << std::endl;
            if( humanAttackPower > SKELETONS ){ humanAttackPower = SKELETONS;}
            SKELETONS = ( SKELETONS - humanAttackPower );
            if( SKELETONS <= 0 ){ SKELETONS = 0; }
            std::cout << "Humans killed " << humanAttackPower << " skeletons!" << std::endl;
        } else {
            std::cout << "Skeletons attack in a fervor!" << std::endl;
            if( skeletonAttackPower > HUMANS ){ skeletonAttackPower = HUMANS; }
            HUMANS = ( HUMANS - skeletonAttackPower );
            if( HUMANS <= 0 ){ HUMANS = 0; }
            std::cout << "Skeletons killed " << skeletonAttackPower << " humans!" << std::endl;
        }
        std::cout << HUMANS << " humans remain" << std::endl;
        std::cout << SKELETONS << " skeletons remain" << std::endl;
    }
    if( HUMANS <= 0 ) {
        std::cout << "The humans have been beaten!" << std::endl;
    }
    if( SKELETONS <= 0 ) {
        std::cout << "The skeletons have been crushed!" << std::endl;
    }

    return 0;
}

IMPROVED CODE

#include <iostream>
#include <stdlib.h>
#include <time.h>

/*
 * Begins the simulation and manages all aspects of the game.
 *
 * @date 10/25/2015
 * @author IWriteThings
 * @return bool
 */
int main() {
    // Initialize the variables.
    int HUMANS, SKELETONS, humanAttackPower, skeletonAttackPower;
    // Initialize the random seed.
    srand (time(NULL));

    // Start the game-simulation
    std::cout << "Number of human combatants:";
    std::cin >> HUMANS;
    std::cout << "Number of skeletons:";
    std::cin >> SKELETONS;

    while( HUMANS > 0 && SKELETONS > 0) {
        humanAttackPower = rand() % HUMANS + 1;
        skeletonAttackPower = rand() % SKELETONS + 1;
        if ((rand() & 1) == 0) {
            std::cout << "Humans attack with gusto!" << std::endl;
            if( humanAttackPower > SKELETONS ){ humanAttackPower = SKELETONS;}
            SKELETONS = ( SKELETONS - humanAttackPower );
            std::cout << "Humans killed " << humanAttackPower << " skeletons!" << std::endl;
        } else {
            std::cout << "Skeletons attack in a fervor!" << std::endl;
            if( skeletonAttackPower > HUMANS ){ skeletonAttackPower = HUMANS; }
            HUMANS = ( HUMANS - skeletonAttackPower );
            std::cout << "Skeletons killed " << skeletonAttackPower << " humans!" << std::endl;
        }
        std::cout << HUMANS << " humans remain." << std::endl;
        std::cout << SKELETONS << " skeletons remain." << std::endl;
    } // End While

    // The win conditions.
    if( HUMANS <= 0 ) {
        std::cout << "The humans have been beaten!" << std::endl;
    } else {
        std::cout << "The skeletons have been crushed!" << std::endl;
    }

    return 0;
} // End main
\$\endgroup\$
3
\$\begingroup\$

Unneeded code

These lines are not necessary:

        if( SKELETONS <= 0 ){ SKELETONS = 0; }
        if( HUMANS <= 0 ){ HUMANS = 0; }

because your previous code already limited the casualties so that the number remaining can never fall below zero.

Clarification because of comment:

Suppose humanAttackPower were 100 and SKELETONS were 30:

        if( humanAttackPower > SKELETONS ){ humanAttackPower = SKELETONS;}
        SKELETONS = ( SKELETONS - humanAttackPower );
        if( SKELETONS <= 0 ){ SKELETONS = 0; }

After the first line, humanAttackPower would be reduced to 30.
After the second line, SKELETONS would be reduced to 0.
The third line is now unnecessary.

Complicated odds

You have a complicated way of determining which side does damage:

    iSecret1 = rand() % 20 + 1;
    iSecret2 = rand() % 20 + 1;
    if( iSecret1 > iSecret2 ) {

What this boils down to is that the humans have a 47.5% chance of doing damage (because ties count towards the skeletons). If that is intentional, I would just do this:

     // 19 out of 40 chances for the humans = 47.5%
     if ((rand() % 40) < 19) {

If you meant the chance to be 50%, then I would do this:

     if ((rand() & 1) == 0) {
\$\endgroup\$
  • \$\begingroup\$ The reason I have the two if statements is because if you remove those, it is possible for it to inform you that there are less combatants than the amount killed. I have to floor to 0 before the check so it doesn't say "30 humans remain. 100 humans were killed" or something similar. If you remove them and run it, you will see what I mean. \$\endgroup\$ – ILikeTurtles Oct 26 '15 at 0:42
  • \$\begingroup\$ How do you know the percentage of chance? I chose 1 - 20 because that is sort of like rolling a dice 20 for initiative in dungeons and dragons. But you are using a different method and know the intricacies of the percent. Do you have a link for that or something? \$\endgroup\$ – ILikeTurtles Oct 26 '15 at 0:54
  • \$\begingroup\$ @Aaron I edited my answer to explain in more detail why your example of 100 and 30 doesn't matter. As far as the percentage goes, it is just basic math. If you generate every possible 1-20 vs every possible 1-20, you will see that skeletons win 210/400 times and humans win 190/400 times. There are 20 ties in 400 combinations, which makes the difference between the two. \$\endgroup\$ – JS1 Oct 26 '15 at 2:53
  • \$\begingroup\$ I have fixed the code using your suggestions. Thank you for all your help. I think it looks a lot better now. \$\endgroup\$ – ILikeTurtles Oct 26 '15 at 6:00
3
\$\begingroup\$
while( HUMANS > 0 && SKELETONS > 0) {
    humanAttackPower = rand() % HUMANS + 1;
    skeletonAttackPower = rand() % SKELETONS + 1;
    if ((rand() & 1) == 0) {
        std::cout << "Humans attack with gusto!" << std::endl;
        if( humanAttackPower > SKELETONS ){ humanAttackPower = SKELETONS;}
        SKELETONS = ( SKELETONS - humanAttackPower );
        std::cout << "Humans killed " << humanAttackPower << " skeletons!" << std::endl;
    } else {
        std::cout << "Skeletons attack in a fervor!" << std::endl;
        if( skeletonAttackPower > HUMANS ){ skeletonAttackPower = HUMANS; }
        HUMANS = ( HUMANS - skeletonAttackPower );
        std::cout << "Skeletons killed " << skeletonAttackPower << " humans!" << std::endl;
    }
    std::cout << HUMANS << " humans remain." << std::endl;
    std::cout << SKELETONS << " skeletons remain." << std::endl;
} // End While

// The win conditions.
if( HUMANS <= 0 ) {
    std::cout << "The humans have been beaten!" << std::endl;
} else {
    std::cout << "The skeletons have been crushed!" << std::endl;
}

Looks decent to me. You could improve the readability (IMHO) by rewriting like this:

static inline int rand(int lo, int hi) {
    return lo + rand() % (hi - lo + 1);
}

static inline bool pct(int p) {
    return rand(1, 100) <= p;
}

// ...

while (humans && skeletons) {
    if (pct(50)) {
        std::cout << "Humans attack with gusto!" << std::endl;
        int attackPower = rand(1, humans);
        int skeletonsKilled = std::min(attackPower, skeletons);
        skeletons -= skeletonsKilled;
        std::cout << "Humans killed " << skeletonsKilled << " skeletons!" << std::endl;
    } else {
        std::cout << "Skeletons attack in a fervor!" << std::endl;
        int attackPower = rand(1, skeletons);
        int humansKilled = std::min(attackPower, humans);
        humans -= humansKilled;
        std::cout << "Skeletons killed " << humansKilled << " humans!" << std::endl;
    }
    std::cout << humans << " humans remain." << std::endl;
    std::cout << skeletons << " skeletons remain." << std::endl;
}

The important differences are:

  • Use lowercase for variables; UPPERCASE in C and C++ tends to be reserved for macros and (sometimes) constants.

  • Eliminate incidental control flow. For example, instead of writing control flow such as "if x is bigger, take y; else take x", just write "Take the min of x and y".

  • Encapsulate random number operations inside helper functions, which are easier to read and will allow you easily to swap in a better RNG if you ever feel the need.

  • Scope incidental variables as tightly as possible. For example, you start by rolling two d20, and then depending on the result of your coin-flip (my pct(50)) you use one or the other of the d20 results. In my version, I flip the coin first, and then I only need to roll one of the d20, since I would have ignored the other d20 anyway.

Of course you could simplify the computation

        int attackPower = rand(1, skeletons);
        int humansKilled = std::min(attackPower, humans);

into

        int humansKilled = std::min(humans, rand(1, skeletons));

if you wanted to. It pretty much just depends on how important the idea of "attack power" is, to the reader of this code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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