generate random numbers and put them in an array

Question

I'm looking to write a little dice game called Farkle (you may know it from Kingdom come delivarance) in C++ but I'm still learning, so I have some trouble with it. atm I'm trying to roll 6 dice and put every rolled number in an array to be able to work with it afterwards. everything seem to work fine but Visual Studio ist outputting this error Code:

Run-Time Check Failure #2 - Stack around the variable 'die' was corrupted.

this is my code:

#include "stdafx.h"
#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>

using namespace std;


void dice() {
    int die[5];
    int i = 1;
    while (i <= 6) {
        die[i] = rand() % 6 + 1;
        cout << die[i];
        i++;
    }
}


int main()
{
    srand(time(NULL));
    dice();
    system("STOP");
    return 0;
}

is ths actually the right approach for this kind of programm?

array of size N have index from 0 to N-1, if int die[N] then loop will be (for int i = 0; i< N; i++) — Arkady Godlin, Apr 10 '18 at 08:21
You are introducing bias by using modulo (`%`). It would be better to use a [std::uniform_int_distribition](http://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution). Also; `rand()` generates pretty poor quality random numbers, a better choice would be [std::mt19937](http://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine) and as for seeding, better use [std::random_device](http://en.cppreference.com/w/cpp/numeric/random/random_device) . — Jesper Juhl, Apr 10 '18 at 08:28
While @ArkadyGodlin intended to explain only, you actually should prefer the for loop in given case for several reasons: 1. scope of `i` is limited to loop body (as you don't need it afterwards anyway) 2. more compact, condition checking and value modification closely together, so you get better readability. — Aconcagua, Apr 10 '18 at 08:30
Possible duplicate of [Why does the indexing start with zero in 'C'?](https://stackoverflow.com/questions/7320686/why-does-the-indexing-start-with-zero-in-c) — Tadeusz Kopec for Ukraine, Apr 10 '18 at 08:34

score 6 · Answer 1 · edited Apr 10 '18 at 08:41

6

No, a better way to generate uniformly distributed random numbers would be

#include <random>
#include <algorithm>

std::random_device rd;  //Will be used to obtain a seed for the random number engine
std::mt19937 gen(rd()); //Standard mersenne_twister_engine seeded with rd()
std::uniform_int_distribution<> d6(1, 6); // {1, 2, 3, 4, 5, 6} with equal probability
int die[5];
std::generate(die, die + 5, [&gen, &d6](){ return d6(gen); });

If you were generating multiple sets of 5d6, you can re-use the same gen rather than re-initialising it each time

edited Apr 10 '18 at 08:41

Aconcagua

24,880
4
34
59

answered Apr 10 '18 at 08:33

Caleth

52,200
2
44
75

Just wondering, if we need to capture `gen`, why don't we `d6`? – Aconcagua Apr 10 '18 at 08:38
@Aconcagua because I wasn't paying attention – Caleth Apr 10 '18 at 08:38
at the moment I just do it for learning purposes, so `srand` seems a little easyer and I don't need much more **but** I'll consider implementing that when I get eveything else to work. Thanks – BiBo Apr 10 '18 at 08:39
@BiBo learning to use appropriate tools is better than learning tools that "seem easy". I strongly suggest using `std::generate` if nothing else – Caleth Apr 10 '18 at 08:43
1

@Caleth while you're right, I don't want to use code that I don't quite understand. as soon I have the basics down, I'll look into that, I still need to learn the syntax a little better because I learned Java before. – BiBo Apr 10 '18 at 08:54

score 3 · Answer 2 · answered Apr 10 '18 at 08:44

As others pointed out. Your error stems from using a too small array. This post will be more about your code being more like C.

It is more idiomatic in C++ to use std::array instead of raw arrays.

Also it is recommended not to use rand() since it produces bad random numbers and by using the modulo operation you are introducing additional bias to you random numbers. Instead one should use the classes from the <random> header.

To make the code even more readable you could try to use the functions from the <algorithm> to replace you loops by named algorithms.

This leads to following code:

#include <algorithm>
#include <array>
#include <iostream>
#include <iterator>
#include <random>

void dice() {
  std::array<int, 6> die;
  std::mt19937 gen{std::random_device{}()};
  std::uniform_int_distribution<int> dice_roll{1, 6};
  std::generate(begin(die), end(die), [&] { return dice_roll(gen); }); 
  std::copy(begin(die), end(die), std::ostream_iterator<int>{std::cout});
}

int main() {
  dice();
  std::cin.get();
}

good, but just a general question for my understanding of c++ : why can you in this case use e.g. `begin` and don't have to type `std::begin`? — JHBonarius, Apr 10 '18 at 08:54
@JHBonarius `die` pulls names in `std::` into consideration because it is a `std::array`. This is called [Argument Dependant Lookup](http://en.cppreference.com/w/cpp/language/adl) — Caleth, Apr 10 '18 at 08:59
@JHBonarius: When a object `x` is defined in some namespace `a` and a function `f` is applied on it `f(x)` then the compiler will search for `f` both in the global namespace and in `a`. As @Caleth wrote, this is called Argument Dependent Lookup. — sv90, Apr 10 '18 at 09:14

Ohad Eytan · Accepted Answer · 2018-04-10T08:32:33.817

2

You have 2 problems in your code:

The size of your array is 5, but you access 6 indices (1 to 6), you can avoid this by changing the <= to < in the condition.
The indices of an array in C++ start with 0, but you start with 1. You can fix that if you change each die[i] to die[i-1] in your code.

Another approach (fixing both problems) would be to initialize i=0 and go with while (i < 5)

edited Apr 10 '18 at 08:32

answered Apr 10 '18 at 08:19

Ohad Eytan

8,114
1
22
31

2

"you can avoid this by change the `<=` to `<` in the condition" (`<` instead of `=`) – Thomas Sablik Apr 10 '18 at 08:21
Sure. Fixed. Thanks – Ohad Eytan Apr 10 '18 at 08:23
1

"Another approach" - well, it is this one to be preferred in given case. `die[i-1]` is fine `i`'s raw value yet is needed for another purpose, but that's not the case here... – Aconcagua Apr 10 '18 at 08:26
Well yeah, if I code this I surely use the second (or most likely a `for` loop), but for learning purpose the first demonstrate (and solve) the problems as well (or even better) – Ohad Eytan Apr 10 '18 at 08:29
@OhadEytan regarding 1. you are wrong here, as he need also to change the while loop value to be <5 and not <6, as his array is of size 5, hence the last valid index is 4 and not 5. – Arkady Godlin Apr 10 '18 at 08:43
2

@OhadEytan oh sorry then, i read them as separate problems. best line is your last line that show the correct solution. – Arkady Godlin Apr 10 '18 at 08:49

score 0 · Answer 4 · answered Apr 10 '18 at 08:16

0

index i should be from 0 to 5, not 1 to 6. It's obvious that when i = 6, it run out of the range of dice which made an error.

Edit these lines:

int i = 0;
    while (i <= 5) {
        ....

answered Apr 10 '18 at 08:16

Chau Pham

4,705
1
35
30

4

`int die[5];` -> index 5 is out of range, too! – Aconcagua Apr 10 '18 at 08:19
@Aconcagua you're right. My mistake. should change dice[6] too – Chau Pham Apr 10 '18 at 09:25

score -2 · Answer 5 · answered Apr 10 '18 at 08:15

Try this code :

#include <iostream>
#include <cstdlib>
#include <ctime>

void populateArray( int ar[], /*const*/ int n )
{
    for( int i = 0 ; i < n ; ++i ) ar[i] = std::rand() % 50 + 1 ;
}

int main()
{
    // http://en.cppreference.com/w/cpp/numeric/random/srand
    std::srand( std::time(nullptr) ) ; // **** important ****

    const int ARRAY_SIZE = 50;
    int ar[ARRAY_SIZE] = {0} ;

    populateArray( ar, ARRAY_SIZE ) ;

    for( int v : ar ) std::cout << v << ' ' ;
}

generate random numbers and put them in an array

5 Answers5