Ocena projektu gier Casino

1. Nie używaj wskaźników i tablic(są wyjątki od tej zasady, ale na razie przyjmij to za pewnik), masz od tego std::array / std::vector
2. Naucz się CMake`a
3. arr[i] = diceRoll(gen); // Generowanie liczby losowej Mogłeś tutaj użyć jakiegoś algorytmu z STL, może std::generate
4. if (decision == 1) Unikaj "magic numbers"
5. for (const auto& pair : frequencyMap) { możesz wykorzystać structure binding
6. w funkcji bool winLose(int a1[], int b1[]) jest jakaś drabinka ifów, której nikt nie przeczyta
7. srand(time(NULL)); rand() Nie używaj tego, masz całą bibliotekę <random>
8. Używaj .clang-format i generalnie używaj formatowania
9. ~Opponent(); Poczytaj rule of 0/3/5, nie wstawiaj destruktora jak go nie potrzebujesz
10. void Player::setValues(string n, int years) Lepiej użyć const ref do dużych typów jak std::string
11. Nie używaj using namespace std
12. char (&arr)[9] Przyznam, że musiałbym wygooglować jak to działa, bo nigdy czegoś takiego nie widziałem w projekcie
13. #ifndef TICTACTOE_H_INCLUDED standardem jest raczej #pragma once, ale można się z tym kłócić
14. extern int chosenField; Tutaj coś poszło nie tak, poczytaj o inline i najlepiej nie używaj zmiennych globalnych, bo doprowadzasz do takich problemów
15. Używaj częściej STL, mignęły mi w projekcie własne implementacje std::max, std::accumulate i innych algorytmów.

Bardzo dziękuję

Ciężko powiedzieć od czego zacząć w ogóle.

1. player.h i pewnie nie tylko - masz losowe wcięcia. Powinno być jednolicie.

2. Po co ci w ogóle pusty destruktor w player.h? W ogóle po co jest ta klasa? Nie wystarczyłoby coś takiego (przy okazji pozbywając się obrzydliwej duplikacji w przypadku Opponent)?

   Kopiuj

   enum class PlayerType {
       MainPlayer,
       Opponent
   }
   
   struct Player {
       const string name;
       const int age;
       const PlayerType type;
       void printInfo(); // notabene, to printInfo nie powinno przyjmować dodatkowych parametrów
   };
   

   a potem używane o tak:

   Kopiuj

   Player p = {"abc", 27, PlayerType::MainPlayer};
   p.name;
   p.age;
   p.printInfo();
   

3. Masz taki kod

   Kopiuj

       if (numberRepeats(arr)==4) {
           std::random_device rd;  // Inicjalizacja generatora losowego
           std::mt19937 gen(rd());  // Inicjalizacja generatora Mersenne Twister
           std::uniform_int_distribution<int> diceRoll(1, 6);  // Zakres liczb losowych od 1 do 6
           arr[findDifferentPosition(arr)] =  diceRoll(gen);
           return 0;
       }
   // a potem
       if (twoPairs(arr) == true) {
           std::random_device rd;  // Inicjalizacja generatora losowego
           std::mt19937 gen(rd());  // Inicjalizacja generatora Mersenne Twister
           std::uniform_int_distribution<int> diceRoll(1, 6);  // Zakres liczb losowych od 1 do 6
           arr[findLeastFrequentIndex(arr)] =  diceRoll(gen);
           return 0;
       }
   
   

   to aż się prosi do wydzielenia do osobnej funkcji.

4. Wiesz co to jest pętla?

   Kopiuj

       sleep(1);
       std::cout << "3.." << std::endl;
       sleep(1);
       std::cout << "2.." << std::endl;
       sleep(1);
       std::cout << "1.." << std::endl;
       sleep(1);
       std::cout << "Let's roll the dice" << std::endl;
   

   1. if (warunek) return true; return false to to samo co return warunek;. Poza tym, zupełnie nie jest czytelne co ta funkcja ma w zamierzeniu robić. Jak masz jakieś skomplikowane warunki, przynajmniej dodaj komentarz opisujący co to robi.
   Kopiuj

   bool twoPairs(int a1[]) {
       if (((a1[0] == a1[1] && a1[2] == a1[3]) || (a1[0] == a1[2] && a1[1] == a1[3]) || (a1[0] == a1[3] && a1[1] == a1[4])) || (a1[1] == a1[2] && a1[3] == a1[4]) || (a1[0] == a1[1] && a1[3] == a1[4]))
           return true;
       return false;
   }
   

5. Funkcja winLose jest obrzydliwa. Nawet nie wiem co dokładnie ma robić, ale przemyśl jeszcze raz co to miało robić, i wyraź prościej te warunki. Być może z pomocą pętli (bo wygląda na to, że chociaż jedna tam by się mogła zdarzyć).

6. Błędna indentacja tutaj: powinieneś zwiększyć wcięcie w środku bloku do while:

   Kopiuj

   
       do {
       while (true) {
           std::cout << "Choose difficulty level: 1 - easy, 2 - hard" << std::endl;
           std::cin >> difficultyLevel;
   
           if (difficultyLevel == 1 || difficultyLevel == 2) {
               break; 
           } else {
               std::cout << "Wrong number! Try again!" << std::endl;
           }
       }
       std::cout << "The player rolls the dice" << std::endl;
       sleep(1);
   

7. int menu() { -> zrób na to enuma, bo zupełnie nie jest potem jasne co te wartości znaczą.

8. void makePlayer (std::string &playerName, int &age, char &gender) {, wbrew temu co nazwa obiecuje, wcale nie tworzy żadnego gracza. Sobie pobiera jakieś wartości, coś tam wypisuje, zapisuje gdzieś wartości, ale wcale gracza nie tworzy. Albo zwracaj gracza (więc nie void makePlayer), albo chociaż przyjmuj Player& jako argument.

9. Zdecyduj się, czy używasz rand(), czy std::random_device

   Kopiuj

       srand(time(NULL));
       int drawRandomWord = rand() % 20;
   

10. Jak nie chcesz tutaj warninga na porównanie zmiennych różnych typów, lepiej zrób size_t i = 0 zamiast tego rzutowania potem.

    Kopiuj

    for (int i = 0; i < int(drawnWord.length()); ++i)
    

11. Znów, wiesz czym jest pętla? A poza tym, isSomeoneWon to niezbyt gramatycznie poprawnie.

    Kopiuj

    bool isSomeoneWon(char (&arr)[9], int& bankrollTictactoe) {
        if ((arr[0] == 'X' && arr[1] == 'X' && arr[2] == 'X') || 
            (arr[3] == 'X' && arr[4] == 'X' && arr[5] == 'X') ||
            (arr[6] == 'X' && arr[7] == 'X' && arr[8] == 'X') ||
            (arr[0] == 'X' && arr[3] == 'X' && arr[6] == 'X') ||
            (arr[1] == 'X' && arr[4] == 'X' && arr[7] == 'X') ||
            (arr[2] == 'X' && arr[5] == 'X' && arr[8] == 'X') ||
            (arr[0] == 'X' && arr[4] == 'X' && arr[8] == 'X') ||
            (arr[2] == 'X' && arr[4] == 'X' && arr[6] == 'X')) {
            std::cout << "Game over. You win!" << std::endl;
            bankrollTictactoe+=5;
            return true;
        }
    

12. if (whoBegin()), to po pierwsze. Po drugie, czemu ewaluujesz whoBegin dwukrotnie? Po trzecie, tam nie ma więcej niż dwóch opcji, a więc zamiast else if powinno być else.

    Kopiuj

        if (whoBegin() == true) {
          // ...
        }
        else if (whoBegin() == false) {
          // ...
        }
    

13. Nie mieszaj języków. Czemu cały Makefile jest po polsku?

Więcej uwag będzie, jak już z tymi się uporasz.

Dziękuję! Jak poprawię to zaktualizuje to wszystko tylko nie wiem kiedy, ale postaram się szybko, bo w pracy mam jeszcze inne rzeczy, a praca nie jest stricte związana z programowaniem, choć czasem się trochę zdarzy.

W CMake bedzie troche czytelniej:

CMakeLists.txt

cmake_minimum_required(VERSION 3.0.0)
project(CasinoGames VERSION 0.1.0 LANGUAGES CXX)

add_executable(CasinoGames 
    main.cpp
    dicePoker.cpp
    gameInfo.cpp
    hangman.cpp
    opponent.cpp
    player.cpp
    tarot.cpp
    ticTacToe.cpp
)

vs Makefile

KATBIEZ=$(shell basename "$(PWD)")
# Nazwy rdzeniowe plikow
GLOWNY = main
PLAYER = player
OPPONENT = opponent
GAMEINFO = gameInfo
TICTACTOE = ticTacToe
HANGMAN = hangman
TAROT = tarot
DICEPOKER = dicePoker
# Po prawej stronie można użyć nazw zmiennych make’a
AR = ar         # archiwizer
AROP = rsv	# Opcje archiwizera
# Tworzenie biblioteki statycznej:
# $@ oznacza nazwę celu, $? –zmodyfikowany prerekwizyt
# Linkowanie: $^ oznacza wszystkie prerekwizyty


# Plik źródłowy
GLOZRO = $(GLOWNY).cpp
ZPLAYER = $(PLAYER).cpp
ZOPPONENT = $(OPPONENT).cpp
ZGAMEINFO = $(GAMEINFO).cpp
ZTICTACTOE = $(TICTACTOE).cpp
ZHANGMAN = $(HANGMAN).cpp
ZTAROT = $(TAROT).cpp
ZDICEPOKER = $(DICEPOKER).cpp
# Pliki skompilowane
GLOKOM = $(GLOWNY).o
OPLAYER = $(PLAYER).o
OOPPONENT = $(OPPONENT).o
OGAMEINFO = $(GAMEINFO).o
OTICTACTOE = $(TICTACTOE).o
OHANGMAN = $(HANGMAN).o
OTAROT = $(TAROT).o
ODICEPOKER = $(DICEPOKER).o
# Plik wykonawczy
GLOWYK = $(GLOWNY).x
# Kompilator
KOMP = g++
# Opcje kompilatora
KOMOP = -Wall -c
# Linker 
LINKER = $(KOMP)
# Opcje linkera
LINOP = -Wall -pedantic
# Regula all - bedzie uruchamiana po wykonaniu make bez argumentow.
# Tu oznacza utworzenie pliku wykonawczego.
all: $(GLOWYK)
# Reguly dla kompilacji:
# 1. Plik z funkcjami
$(OPLAYER): $(ZPLAYER) 
	$(KOMP) $(KOMOP) $(ZPLAYER)
$(OOPPONENT): $(ZOPPONENT) 
	$(KOMP) $(KOMOP) $(ZOPPONENT)
$(OGAMEINFO): $(ZGAMEINFO) 
	$(KOMP) $(KOMOP) $(ZGAMEINFO)
$(OTICTACTOE): $(ZTICTACTOE) 
	$(KOMP) $(KOMOP) $(ZTICTACTOE)
$(OHANGMAN): $(ZHANGMAN) 
	$(KOMP) $(KOMOP) $(ZHANGMAN)
$(OTAROT): $(ZTAROT) 
	$(KOMP) $(KOMOP) $(ZTAROT)
$(ODICEPOKER): $(ZDICEPOKER) 
	$(KOMP) $(KOMOP) $(ZDICEPOKER)
$(GLOKOM): $(GLOZRO) 
	$(KOMP) $(KOMOP) $(GLOZRO)
# Po prawej stronie można użyć nazw zmiennych make’a
AR = ar         # archiwizer
AROP = rsv	# Opcje archiwizera
# Tworzenie biblioteki statycznej:
# $@ oznacza nazwę celu, $? –zmodyfikowany prerekwizyt
# Linkowanie: $^ oznacza wszystkie prerekwizyty
$(GLOWYK): $(GLOKOM) $(OPLAYER) $(OOPPONENT) $(OGAMEINFO) $(OTICTACTOE) $(OHANGMAN) $(OTAROT) $(ODICEPOKER)
	$(LINKER) -o $@ $(LINOP) $^