Pasjans w C++, pytanie o styl

0

Napisałem kolejny program, który tym razem gra w dość głupiego i prostego pasjansa. Zgodnie z sugestiami forumowiczów zmienne i funkcje mają angielskie nazwy. Funkcje pozamykałem w klasę. Ale nie wiem czy to dobrze wyszło. Proszę o konstruktywną krytykę.


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

unsigned seed = std::chrono::system_clock::now().time_since_epoch().count();
std::default_random_engine gen(seed);


class Pasjans {
    std::vector<int> deck;
    std::vector<int> table[2];
    int howmanycards{1};
    uint64_t successTOTAL{1};
    uint64_t failureTOTAL{1};
    double successratio{0};
    int flag{0};
    int Lays{0};


    void ToggleFlag(int &flag) {
        flag = !flag;
    }

    void ShuffleDeck(std::vector<int> &deck, int howmanycards) {
        int howmanyfigures = howmanycards / 4;
        for (int i = 0; i < howmanycards; i++) deck.push_back(i % howmanyfigures);
        std::shuffle(deck.begin(), deck.end(), gen);
    }

    void LayDownCard(std::vector<int> &deck, std::vector<int> table[], int flag) {
        table[flag].push_back(*(deck.end() - 1));
        deck.pop_back();
    }

    void ShiftCard(std::vector<int> table[], int flag) {
        table[flag].push_back(*(table[!flag].end() - 1));
        table[!flag].pop_back();
        CompareCards(table, flag);
        ToggleFlag(flag);
    }

    void TakeTwoCards(std::vector<int> table[], int flag) {
        table[flag].pop_back();
        table[flag].pop_back();
        ShiftCard(table, flag);
    }

    void CompareCards(std::vector<int> table[], int flag) {
        if (table[flag].size() >= 2)
            if (*(table[flag].end() - 1) == *(table[flag].end() - 2)) {
                TakeTwoCards(table, flag);
            }
    }

    bool CollectCards(std::vector<int> table[], std::vector<int> &deck) {
        if (table[0].size() > 1) {
            for (unsigned int i = 0; i < table[0].size(); i++) deck.push_back(table[0][i]);
            table[0].clear();
            for (unsigned int i = 0; i < table[1].size(); i++) deck.push_back(table[1][i]);
            table[1].clear();
            return true;
        } else return false;
    }

public:
    void Play() {
        std::cout << std::fixed;
        while (howmanycards % 4) {
            std::cout << "Podaj liczbe kart podzielna przez 4:\n";
            std::cin >> howmanycards;
        }
        while (std::cin.get() != '\n') continue;
        for (int rounds = 0; rounds < 10000000; rounds++) {
            ShuffleDeck(deck, howmanycards);
            do {
                while (!deck.empty()) {
                    LayDownCard(deck, table, flag);
                    CompareCards(table, flag);
                    ToggleFlag(flag);
                }
                Lays++;
            } while ((CollectCards(table, deck)) && (Lays < howmanycards));
            if (table[0].size() == 1) successTOTAL++; else failureTOTAL++;
            successratio = (double) successTOTAL / (double) failureTOTAL;
            if (!(rounds % (4000000 / howmanycards)))
                std::cout << "Success ratio: " << successratio << " Rounds: " << rounds << "\n";
            table[0].clear();
            table[1].clear();
            deck.clear();
            Lays = 0;
        }
    }
};

int main() {
    Pasjans obiekt;
    obiekt.Play();
    return 0;
}

1

std::vector<int> table[2];
Lepiej mieć Vector vectora niż bawić się w tablice ;) a jezeli znana jest wielkosc kontenera to uzyc std::array lub jakas klase na to osobna (strukture)
Przeczytaj komentarz pod tym postem.

Czasem zmienne są camelcase a czasem wszystko z małych. Najlepiej zdecydować się na jeden styl (proponuje ten który jest polecany dla c++)

Magic numbers

Ogólnie dobrze napisany kod jak na początkującego:)

1
  1. lepiej by było gdyby nazwy zmiennych lokalnych też były camelCasem lub snake_casem. Czyli U Ciebie np. howmanycards mogło by być how_many_cards. Myślę, że tak by było bardziej czytelnie.
  2. nie podoba mi się, że zmienna flag jest liczbą całkowitą i jest używana jako indeks. Flag raczej powinien być typu bool, a jeśli to jednak ma działać jako indeks do tablicy to zmienił bym nazwę.
  3. zamiast table[!flag].end() - 1 można table[!flag].rbegin()
  4. Zauważyłm, że jako parametry metod przekazujesz pola klasy. Nie trzeba tego robić. Metody klasy jako pierwszy niewidoczny parametr przekazują wskaźnik this, więc wszystkie pola klasy są widoczne.
  5. Dodałbym konstruktor i tam zainicjalizował zmienne.

Ogólnie zgadzam się, że kod dobrze napisany jak na początek :)

1

Nie szedłbym na tym etapie w klasy, bo trochę mącisz i nie czerpiesz z klasy nic, a nic tego co jest w nich najlepsze.

Na moje oko Twój kod teraz wykonuję pracę jak zwykła procedura tyle, że jest zapisany jako klasa. Masz tam co prawda prywatne pola, które są lepsze niż globalne. Niby to dobrze, ale skoro wszystkie metody w dowolnym momencie mają dostęp do wszystkich pól to efekt jest bez różnicy. Inaczej mówiąc weź spróbuj na tym etapie wytłumacz komuś dlaczego globalne są gorsze niż pola w Twojej klasie :-)

Jak chcesz zrobisz to samo co teraz próbujesz uzyskać poprzez tą klasę (czyli ukryć dane) to najprościej będzie stworzyć zwykłą funkcje która będzie mieć dane zapisane jako lokalne, w ten sposób ukryjesz dane i określisz zakres ich użycia. Efekt w zasadzie ten sam, ale wtedy to Twoja funkcja będzie określać widoczność danych dla funkcji pomocniczych.

void play() {
    // tu tworzę wszystkie pola jak lokalne trochę jak w starym C czy Pascalu :-)
    vector<int> a;
    int b = 0;
    bool ok = false;

    // a później na nich wołam pomocnicze funkcje
    h1(a);
    // ... jakiś kod i wtedy
    h2(b);
    h3(a, b, ok);

   if (ok) {
     print(..)
   }
}

Inna rzecz to skup się trochę bardziej na swoich funkcjach, bo sposób obsługi danych w nich robisz na około. Dużo modyfikujesz, niemal każdy argument gdzieś po drodze zmieniasz i łatwo wtedy coś przeoczyć, np. w ShiftCard wołasz ToggleFlag, metoda ToggleFlag mutuje po referencji, ale argument jaki otrzymałeś w ShiftCard był kopią flagi, więc ta modyfikacja nic tak naprawdę nie zmienia.

Jesli miałbym dać Ci tutaj wskazówkę to propozycja jest taka: korzystaj z własności funkcji. Funkcje powinny bronić się, wystrzegać wpływu na swoje argumenty, bo wtedy są dużo bardziej nieprzewidywalne. Zamiast tego korzystaj z faktu, że funkcje mogą zwrócać wynik, buduj na tym swój program, bo to jest dużo bardziej przewidywalny sposób pracy. Wołasz funkcje, ona Ci daje wynik i potem coś na nim robisz to jest lepsze niż dajesz funkcji coś i ta funkcja może tak naprawdę wszystko z tym zrobić, osoba analizująca Twój kod musi wtedy zaglądać do każdej Twojej funkcji by wiedzieć o co tu chodzi.

Jak zaczniesz produkować wartości, zyskasz kolejny punkt oparcia. Mianowicie będziesz mógł określać łatwiej czas życia zmiennych, Jak z czasem zauwazysz, jeśli zaczniesz zwracać uwagę na to, że funkcje mogą coś produkować to pewnym zmiennym będziesz mógł zawezić czas życia, opóźnić czas tworzenia, a nawet uniknąć jeśli sytuacja nie będzie tego wymagać. Dobrą praktyką jest ogólnie tworzyć zmienne tak późno jak to możliwe (więc wtedy ten pseudoprzykład jaki Ci dałem wyżej można sprowadzić do prostszej postaci)

void play() {
    vector<int> a = h1();
    int b = h2();
    bool ok = h3(a, b);
     if (ok) {
       print(..)
    }
}

Po co to wszystko?

By jak najmniej mieć na głowie, bo jak jest wiele rzeczy, które możesz zmienić i jak nie ma żadnych reguł, które mogłby powstrzymać Cię przed tym to uzyskasz środowisko w którym bardzo łatwo jest tworzyć trudny w zrozumieniu kod (nawet jesli piszesz coś trywialnego). Reasumując, jesli chcesz uczyć się porządnie pisać kod to powinieneś zwracać uwagę na te elementy, które upraszczają Ci pracę, a te konstrukcje, które umożliwiają robić Ci wszystko poddawać wątpliwości.

0

To nie jest kwestia stylu, o który pytasz, ale meteorytyki, ale uznałem, że i tak warto o tym wspomnieć — źle seedujesz swój generator pseudolosowy.

A co jest nie tak w Twoim rozwiązaniu? Seedujesz 32-bitową wartością generator pseudolosowy, który może mieć znacznie większy stan. Jest zresztą bardzo prawdopodobne, że domyślnym będzie mt19937, który ma właśnie te 19 937 bitów stanu. Czyli, innymi słowy, wypełniasz 0,16% tego, co powinieneś wypełnić. To da Ci absolutnie fatalne własności statystyczne.

Ogólnie, jeśli nie masz ku temu dobrych powodów (jak np. konieczność zapewnienia powtarzalnych rezultatów), powinieneś seedować tylko i wyłącznie przez std::random_device.

A co zrobić, jakbyś miał ten dobry powód? Wtedy powinieneś użyć albo generatora o mniejszym stanie (i tak, poprawnie zaseedowany nawet najprostszy 32-bitowy LCG będzie dawał znacząco lepsze rezultaty, niż źle zaseedowany Mersenne Twister), albo seeda porównywalnych rozmiarów, co stan generatora — czyli zamiast jednego 32-bitowego inta potrzebowałbyś ich jakoś z 624…

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