Skip to main content
added 256 characters in body
Source Link
Jerry Coffin
  • 34.1k
  • 4
  • 77
  • 145

So, even though it's more code than I'd like, using it is pretty easy (even easier than srand()/rand()), and you get much better random number generation. Oh, one other minor point: as it stands now, this depends on a C++ 17 feature that deduce the template parameter from the value passed to the constructor. For compilers that don't implement that yet, change generator g(100); to generator<int> g(100);.

So, even though it's more code than I'd like, using it is pretty easy (even easier than srand()/rand()), and you get much better random number generation.

So, even though it's more code than I'd like, using it is pretty easy (even easier than srand()/rand()), and you get much better random number generation. Oh, one other minor point: as it stands now, this depends on a C++ 17 feature that deduce the template parameter from the value passed to the constructor. For compilers that don't implement that yet, change generator g(100); to generator<int> g(100);.

added 630 characters in body
Source Link
Jerry Coffin
  • 34.1k
  • 4
  • 77
  • 145

This accomplishes the job rather more easily.

srand/rand must go

Though using them is somewhat more work, I'd prefer to use the random number generators in random instead of srand/rand. They're generally of considerably higher quality, and they already provide code to handle creating random numbers within a range. I've spent a while working on creating a wrapper to make it somewhat easier to use these, and come up with the following:

#pragma once
#include <array>
#include <random>
#include <algorithm>

class generator {
    template <class Rand> 
    class Seed {
        class seeder {
            std::array < std::random_device::result_type, Rand::state_size > rand_data;
        public:
            seeder() {
                std::random_device rd;
                std::generate(rand_data.begin(), rand_data.end(), std::ref(rd));
            }
    
            typename std::array < std::random_device::result_type, Rand::state_size >::iterator begin() { return rand_data.begin(); }
            typename std::array < std::random_device::result_type, Rand::state_size >::iterator end() { return rand_data.end(); }
        } seed;
    
        std::seed_seq s;
    
    public:
        Seed() : s(seed.begin(), seed.end()) { }
    
        template <class I>
        auto generate(I a, I b) { return s.generate(std::forward<I>(a), std::forward<I>(b)); }
    };

    using Rand = std::mt19937_64;
    Seed<Rand> seed;
    Rand rng;
    std::uniform_int_distribution<int> uni;
public:
    generator(int high) : rng(seed), uni(0, high) {}
    generator(int low, int high) : rng(seed), uni(low, high) { }
    int operator()() { return uni(rng); }
};

I realize this is a bunch of code that you probably don't want to deal with right now. To keep it simple to use, it's written as a header, so you basically just do something like:

#include "rand.h"

int main() { 
    generator g(100);    // create a generator for numbers from 0 to 100

    // generate and print out some numbers:
    for (int i = 0; i < 100; i++) {
        std::cout << g() << "\t";
        if (i % 10 == 0)
            std::cout << "\n";
    }
}

So, even though it's more code than I'd like, using it is pretty easy (even easier than srand()/rand()), and you get much better random number generation.

This accomplishes rather more easily.

This accomplishes the job rather more easily.

srand/rand must go

Though using them is somewhat more work, I'd prefer to use the random number generators in random instead of srand/rand. They're generally of considerably higher quality, and they already provide code to handle creating random numbers within a range. I've spent a while working on creating a wrapper to make it somewhat easier to use these, and come up with the following:

#pragma once
#include <array>
#include <random>
#include <algorithm>

class generator {
    template <class Rand> 
    class Seed {
        class seeder {
            std::array < std::random_device::result_type, Rand::state_size > rand_data;
        public:
            seeder() {
                std::random_device rd;
                std::generate(rand_data.begin(), rand_data.end(), std::ref(rd));
            }
    
            typename std::array < std::random_device::result_type, Rand::state_size >::iterator begin() { return rand_data.begin(); }
            typename std::array < std::random_device::result_type, Rand::state_size >::iterator end() { return rand_data.end(); }
        } seed;
    
        std::seed_seq s;
    
    public:
        Seed() : s(seed.begin(), seed.end()) { }
    
        template <class I>
        auto generate(I a, I b) { return s.generate(std::forward<I>(a), std::forward<I>(b)); }
    };

    using Rand = std::mt19937_64;
    Seed<Rand> seed;
    Rand rng;
    std::uniform_int_distribution<int> uni;
public:
    generator(int high) : rng(seed), uni(0, high) {}
    generator(int low, int high) : rng(seed), uni(low, high) { }
    int operator()() { return uni(rng); }
};

I realize this is a bunch of code that you probably don't want to deal with right now. To keep it simple to use, it's written as a header, so you basically just do something like:

#include "rand.h"

int main() { 
    generator g(100);    // create a generator for numbers from 0 to 100

    // generate and print out some numbers:
    for (int i = 0; i < 100; i++) {
        std::cout << g() << "\t";
        if (i % 10 == 0)
            std::cout << "\n";
    }
}

So, even though it's more code than I'd like, using it is pretty easy (even easier than srand()/rand()), and you get much better random number generation.

added 630 characters in body
Source Link
Jerry Coffin
  • 34.1k
  • 4
  • 77
  • 145

Two Step Initialization

Your Game class has a create member function, which looks like a design smell to me. To create a Game you should just...create a Game. Creating a Game object, then calling create on that object to really create an actual game is two-step initialization, which you usually want to avoid. Generally speaking, if you need to do something to create an object, that should be done in the constructor.

Unnecessary Switch

It looks to me like this switch statement:

    switch (ch)
    {
    case Left:
        moveCells(Left);
        break;
    case Right:
        moveCells(Right);
        break;
    case Up:
        moveCells(Up);
        break;
    case Down:
        moveCells(Down);
        break;
    }
}

...is unnecessarily long and kind of pointless. For the most part, it's equivalent to just: moveCells(ch);, though you may need to assure that ch is one of Up, Down, Left or Right first. One possibility (that still uses a switch statement) would be:

switch (ch) {
case Up:
case Down:
case Left:
case Right:
    moveCells(ch);
}

This accomplishes rather more easily.

Two Step Initialization

Your Game class has a create member function, which looks like a design smell to me. To create a Game you should just...create a Game. Creating a Game object, then calling create on that object to really create an actual game is two-step initialization, which you usually want to avoid. Generally speaking, if you need to do something to create an object, that should be done in the constructor.

Unnecessary Switch

It looks to me like this switch statement:

    switch (ch)
    {
    case Left:
        moveCells(Left);
        break;
    case Right:
        moveCells(Right);
        break;
    case Up:
        moveCells(Up);
        break;
    case Down:
        moveCells(Down);
        break;
    }
}

...is unnecessarily long and kind of pointless. For the most part, it's equivalent to just: moveCells(ch);, though you may need to assure that ch is one of Up, Down, Left or Right first. One possibility (that still uses a switch statement) would be:

switch (ch) {
case Up:
case Down:
case Left:
case Right:
    moveCells(ch);
}

This accomplishes rather more easily.

added 630 characters in body
Source Link
Jerry Coffin
  • 34.1k
  • 4
  • 77
  • 145
Loading
Source Link
Jerry Coffin
  • 34.1k
  • 4
  • 77
  • 145
Loading