Proszę o weryfikację kodu, generator zestawów liczb do lotto

0

Uczę się programować w C++, (wcześniej programowałem w Ruby). Testowałem na algorytmie losowania bez powtórzeń różne funkcje języka, ostatecznie jest to losowanie bez powtórzeń elementów tablicy o dowolnych elementach, udało mi się wyciągnąć wydajność samego algorytmu o około 20ms (1000 liczb) gorszą od tej ze strony algorytm.org (przy tym samym generatorze liczb pseudolosowych i wyrzuceniu wyświetlania wylosowanych liczb na oddzielną pętlę). Do generowania liczb pseudolosowych używam mocniejszego generatora liczb pseudolosowych niż ten z funkcji rand() (używam Mersene Twister 19937 generator (64 bit)). Program jest na Windowsa ponieważ używa polecenia systemowego zmiany strony kodowej cmd.exe.
Proszę o sprawdzenie poprawności kodu, zaznaczam że uczę się od soboty. Sam program generuje zestawy liczb do lotto, ale po modyfikacji może być użyty w dowolnym innym celu np. tasowania stablicowanych zestawów danych.

#include <iostream>
#include <thread>
#include <random>
#include <vector>
#include <chrono>
#include <algorithm>

//g++ -std=c++1y -static-libgcc -static-libstdc++ -o Lotto.exe ./lotto.cpp

template<typename Type>
class Random
{
    int random_index;
    Type element;
    std::vector<Type> result;
    int get_element(int);
public:
    std::vector<Type> shuffle(std::vector<Type>, int, int);
};

template<typename Type>
int Random<Type>::get_element(int counter)
{
    static std::random_device random_device;
    static size_t time = static_cast<size_t>(std::time(0));
    static size_t clock = static_cast<size_t>(std::clock());
    static size_t pid = std::hash<std::thread::id>()(std::this_thread::get_id());
    static std::seed_seq seed_value = {time, clock, pid};
    static std::mt19937_64 random_number_generator(seed_value);
    std::uniform_int_distribution<int> element(0, counter);
    return element(random_number_generator);
}

template<typename Type>
std::vector<Type> Random<Type>::shuffle(std::vector<Type> vector, int max, int max_shuffled)
{
    for (int i = 0; i < max_shuffled; max_shuffled--)
    {
        random_index = get_element(max - 1);
        element = vector[random_index];
        result.push_back(element);
        vector.erase(vector.begin() + random_index);
        max--;
    }
    return result;
}

int main()
{
    system("chcp 65001 > nul");
    std::vector<int> input;
    int max, max_shuffled;
    int last;
    std::cout << "Podaj maksymalną liczbę do wylosowania: ";
    std::cin >> max;
    std::vector<int> output;
    std::cout << "Podaj ile liczb wylosować: ";
    std::cin >> max_shuffled;
    std::cout << std::endl;
    for (int i = 1; i < max + 1; i++)
    {
        input.push_back(i);
    }
    std::cout << "Zestaw:";
    std::cout << "\t\tLiczby:" << std::endl;;
    for (int s = 1; s <= 10; s++)
    {
        std::this_thread::sleep_for(std::chrono::milliseconds(1));
        Random<int> get_shuffled;
        std::cout << "\t" << s;
        std::cout << "\t\t";
        output = get_shuffled.shuffle(input, max, max_shuffled);
        last = output[max_shuffled - 1];
        std::sort(output.begin(), output.end());
        for (int i = 0; i < max_shuffled; i++)
        {
            if (output[i] < 10)
            {
                std::cout << "0" << output[i] << ", ";
            }
            else
            {
                std::cout << output[i] << ", ";
            }
        }
       
        std::cout << "Ostatnia: ";
        if (last < 10)
        {
            std::cout << "0" << last << std::endl;
        }
        else
        {
            std::cout << last << std::endl;
        }
    }
    std::cout << std::endl;
    std::cout << "Koniec." << std::endl;
    char block;
    std::cin >> block;
    return 0;
}

0

Za mała entropia na wejściu. Pid + czas to trochę za mało. Może wartałoby użyć /dev/urandom?

3

Kod jest dość słaby, ale jeśli uczysz się tak krótko to i tak całkiem nieźle.

  1. Niepotrzebne zmienne globalne. Jeśli nie chcesz przekazywać RNG/PRNGdo każdego wywołania funkcji to użyj zmiennych thread_local, aby uniknąć wyścigów.
  2. przyjmujesz parametry przez kopię. Dla takiego std::vector kopia może być bardzo kosztowna.
  3. wymyślasz koło na nowo. [std::shuffle][1] is a thing
  4. Nie wykorzystujesz zmiennych (np. random_device).
  5. Tworzysz osobną instancję prng dla każego typu Random
  6. Stosujesz przesłanianie nazw (element)
  7. nie używasz <iomanip>, zaśmiecając tym samym kod.

Wywaliłbym cały szablon Random. Nie jest on do niczego potrzebny. Ewentualnie możesz argumentować, że Twoja implementacja shuffle jest inna od standardowej ale:

  1. powinna mieć w takim razie inną nazwę
  2. nie potrzebuje do tego szablonu klasy

Inicjalizację PRNG bym zrobił tak (lekko wzorując się na Twoim kodzie):

thread_local std::mt19937_64 prng = []{
	std::seed_seq seq{
		static_cast<std::uint32_t>(std::hash<std::thread::id>{}(std::this_thread::get_id())),
		static_cast<std::uint32_t>(std::time(nullptr)),
		std::random_device{}(),
		std::random_device{}(),
		std::random_device{}()
	};
	return std::mt19937_64{seq};
}();

wtedy random(min, max) można prosto zaimplementować jako:

template<typename T>
T random(T min, T max) {
	return std::uniform_int_distribution<T>{min, max}(prng);
}

I wtedy w main() zamiast output = get_shuffled.shuffle(input, max, max_shuffled);:

auto output = input;
std::shuffle(output.begin(), output.end(), prng);
output.resize(max_shuffled);

Ponadto:

            if (output[i] < 10)
            {
                std::cout << "0" << output[i] << ", ";
            }
            else
            {
                std::cout << output[i] << ", ";
            }

Można zapisać jako

std::cout << std::setw(2) << std::setfill('0') << output[i] << ", ";

Idąc dalej:

    std::vector<int> input;
//...
    for (int i = 1; i < max + 1; i++)
    {
        input.push_back(i);
    }

	std::vector<int> input(max);
	std::iota(input.begin(), input.end(), 1);

Zbierając to do kupy otrzymasz kod prawie 2x krótszy:
http://melpon.org/wandbox/permlink/bzdEEa4W2G4yJaIm
[1]: http://en.cppreference.com/w/cpp/algorithm/random_shuffle

1 użytkowników online, w tym zalogowanych: 0, gości: 1