2
\$\begingroup\$

I changed the program to use a dynamic array instead of a vector or storing the integers. This program works without bugs and executes relatively fast. The only doubt Istill have is how to fill the dynamic array in am ore elegant fashion (I couldn't get array={0} or array.fil(0) to work.

At this point, there's to assess the program efficiency.


  1. It takes three inputs: a reference value, a pool of allowed attempts and a control value for the loop controlling the generation of data

  2. It attempts to surpass a reference value with a call to rand a certain number of times

  3. It stores in a float value how many successes per number of attempts it had, exits then launches a new session until the control value has expired

  4. It stores those float numbers in a vector (for example: 5 attempts will produce decimal versions of fifths as in \$ (0.0, 0.0, 0.2, 0.2, 0.2, 0.2, 0.4)\$, 10 attempts will produce tenths as in \$ (0.0, 0.1, 0.1, \ldots)\$)

  5. It stores those floats in an integer array by translating the decimal values in integers based on their ratio with the number of attempts and then use this value as their index, for example \$ (0.2, 0.4, 0.4, 0.6)\$ with a pool of 5 becomes \$ (0, 1, 2, 1, \ldots)\$, with a pool of 10 becomes \$ (0, 0, 1, 0, 2, 0, 1, \ldots)\$

  6. It presents the data of the integer array on the console


#include <iostream>
#include <iomanip>      /* std::setw */
#include <limits>       /* std::numeric_limits */
#include <string>
#include <vector>
#include <algorithm>    /* std::sort */

#include <stdlib.h>     /* srand, rand */
#include <time.h>       /* time */

void inputInterface(int&, int&, unsigned int&);
void chargeResult(const int&, const int&, unsigned int&, std::vector<float>&, unsigned int*);
void chargeResultArr(const int&, std::vector<float>&, unsigned int*);
float attempt(const int&, const int&);
void outputArray(const int, const unsigned int*);

int main()
{
    int difficulty = 0, pool = 0;
    unsigned int control = 0;
    std::vector<float> resultList;
    unsigned int *resultListByValue;

    //generate the random seed
    srand(time(NULL));

    //input variables and plugs one of them as size of the integer array
    inputInterface(difficulty, pool, control);
    resultListByValue = new int[pool + 1];
    for (int i = 0; i < pool + 1; i++) resultListByValue[i] = 0;

    //runs the numbers and charges them to the float vector as enumeratiom then to the integer array as sorted data 
    chargeResult(difficulty, pool, control, resultList, resultListByValue);

    //outputs the data
    outputArray(pool, resultListByValue);
    delete[] resultListByValue;

    system("pause");

    return 0;
}

void inputInterface(int& diff, int& pool, unsigned int& cont)
{
    using namespace std;

    cout << "difficulty (on 100): "; cin >> diff;
    cout << "attempts: "; cin >> pool;
    //cont = numeric_limits<unsigned int>::max();
    cout << "control: "; cin >> cont;
}

void chargeResult(const int& diff, const int& pool, unsigned int& cont, std::vector<float>& resList, unsigned int* resListArr)
{
    using namespace std;

    float result = 0.0;

    //runs the simulation and stores each result in a vector of size equal to {int control}
    //sorts the numbers NOTE: remember that a pool of n produces numbers that are multiples of A/n in a range [0, 1]
    //example: pool=5, context=4, vector={0.2, 0.4, 0.4, 0.6}
    while (cont--) {
        if ((result = attempt(diff, pool)) >= 0) {
            if (resList.size() > (1024 / sizeof(float))) {
                sort(resList.begin(), resList.end());
                chargeResultArr(pool, resList, resListArr);
                resList.clear();
            }
            resList.push_back(result);
        }       
    }
    sort(resList.begin(), resList.end());
    chargeResultArr(pool, resList, resListArr);    

    resList.clear(); vector<float>(resList).swap(resList);
}

void chargeResultArr(const int& pool, std::vector<float>& resList, unsigned int* resListArr)
{
    using namespace std;

    float myValue = -1.0;
    int pos;
    //iterates the float vector, when th enext value is different from the preceding one, it iterates the integer array to that number times {pool}
    //NOTE: this means multiplying A/n * n = A
    //NOTE: the array can accomodate {pool+1} values so that it has space for results from "0/n" to "n/n"
    //example: vector={0.2, 0.4, 0.4, 0.6}, array={0, 1, 2, 1, 0, 0}
    for (vector<float>::iterator it = resList.begin(); it != resList.end(); it++) {
        if (myValue != *it)
            pos = (int)(pool * *it);
        resListArr[pos]++;
        myValue = *it;

    }
}

float attempt(const int& diff, const int& pool)
{
    int r = 0, rolls = 0, wins = 0;

    //runs random numbers against a target value and keeps track of the number of attempts and the number of successes
    for (; rolls < pool; rolls++) {
        (r = rand() % 100 + 1);
        if (r >= diff) wins++;
    }

    //stores attempts and successes in a float
    //NOTE: a pool of n will produce A/n fractions in decimal form, also the range is [0, 1]
    return ((float)wins / (float)rolls);

}

void outputArray(const int pool, const unsigned int* myArray)
{
    using namespace std;

    //if an array position has a positive value, it outputs the position and the vale
    //example: array={0, 1, 2, 1, 0, 0}, "1 success on 4"= 1 time, "2 successes on 4" = 2 times, "3 successes on 4" = times
    for (int i = 0; i < pool + 1; i++)
        if (myArray[i] > 0)
            cout << i << "/" << pool << "\t|\t" << setw(10) << myArray[i] << endl;
}
\$\endgroup\$
  • \$\begingroup\$ I modified the program to discharge the enumeration vector to the sorted array whenever the vector reaches a fixed size, this allows to contain the amount of memory required by the program. I arbitrarily set this size to 1mb, but I would welcome consideration on how to manage this size dinamically to balance reserved memory and execution time. \$\endgroup\$ – maja Mar 27 '16 at 11:11
1
\$\begingroup\$

What Does The Code Do?

I have to admit that I'm unclear on what this code is supposed to be doing. It appears that the attempt() method is randomly picking a number, then checking if that number is greater or equal to another number. It keeps a count of the ratio of times it successfully meets or beats the given number vs. the number of rolls. And main() repeats this process several times, keeping track of the resulting ratio of "wins" to "rolls". Is this some sort of Monte Carlo process?

None of the words in the code describe to me what's actually happening or why you would want to do any of this. What is the significance of keeping track of wins vs. rolls? And what are you doing with the integer array? And why is it an integer array instead of a float array? None of this is clear from the code, nor does your description above it make it any clearer. Which leads to the next point:

Work on Naming Things

I don't see any concrete words in the code. Usually code will have some words that represent real-world concepts, such as numStudents, or transactionTotal, or playerPosition. The single function you have is named attempt(). Attempt what? What is it attempting to do? I can read the code to see what it does, but I don't understand why you want to do that. Is it simulating something? Is it calculating something?

At the very least, I'd rename pool to numRollsPerAttempt, although it would be even better to have it say what it's attempting to do, such as numDiceRollsPerGame or numSimulatedJavelinThrows or whatever it actually represents. I'd rename control to numSimulations or numGames or something that evokes what you're actually trying to achieve. And contest would probably be better named as breakpoint, threshold, or minWinValue. Something that explains what it's for.

Break Things Into Functions

Your main() function does several things:

  1. Gets input from the user
  2. Simulates lots of dice rolls
  3. Generates some statistics about them
  4. Displays the results

These should each be separate functions.

Don't Use Namespace std

In general, it's a bad idea to have using namespace std at the top of your source file. See here for a much more detailed explanation than I could give.

Readability

I'm finding some of your code very difficult to read. This line in particular:

(myValue == *it) ? resultListByValueArr[pos]++ : resultListByValueArr[pos = (int)(pool * *it)]++;

There's so much going on in that one line of code, and it doesn't gain you anything. In that code you have:

  1. a conditional that's phrased very oddly
  2. a dereference of a value in an array
  3. an increment of that value
  4. a difficult to see assignment
  5. a multiplication
  6. another dereference (which relies on the assignment and the multiplication)
  7. another increment

It would be much clearer to the reader and just as efficient to write:

if (myValue != *it)
{
    pos = (int)(pool * *it);
}
resultListByValueArr[pos]++;

It's 3 lines of code instead of 1, but is exactly the same thing. Don't make it harder on future readers of your code (including yourself in 6 months!).

Potential Errors

I see several places that are likely to become errors if the code is ever modified in the future:

You have several single line while and if statements that you don't put curly braces around. This is valid C/C++, but could cause bugs in the future if someone comes along and attempts to add a line to one of them without realizing the braces aren't there. I recommend braces around all conditionals and loops regardless of whether they're 1 statement long or not.

The variable pos is not initialized. It happens to work out in this case because the first time through the loop, the thing pointed to by it can't be -1.0, but if the logic ever changes things could go very badly. You should initialize pos to either 0 or whatever an appropriate first value could be.

\$\endgroup\$
  • \$\begingroup\$ I have edited the program to use external functions and I have added many explanations, including examples. about the curly braces, I find that adding too many of them detracts more to readability then just having simple statements follow each other. (if this was meant to go into productions I would still add all of them.. ) \$\endgroup\$ – maja Mar 27 '16 at 9:07
  • \$\begingroup\$ about what it does: it takes a difficulty (in percent), runs it a given amount of times and then creates a distributions of how many attempts were successful. \$\endgroup\$ – maja Mar 27 '16 at 9:09

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.