C ++ Number Guessing Game

Question

Ich habe ein einfaches Ratespiel erstellt, bei dem der Spieler wählen kann, ob der Spieler die Zahl oder den Computer errät.

Wenn der Spieler die Zahl errät, generiert der Computer eine Zufallszahl zwischen 1 und 100. Dann muss der Spieler die Zahl des Computers erraten.

Zuerst gibt der Spieler seine erratene Nummer ein. Wenn es zu hoch als die Nummer des Computers ist, druckt das Programm aus, dass die Nummer des Players zu hoch ist, wenn sie zu niedrig ist, und umgekehrt.

Wenn es korrekt ist, gratuliert der Computer dem Spieler und fragt, ob der Spieler erneut spielen möchte oder nicht. Wenn der Player erneut spielen möchte, wird das Programm neu gestartet. Wenn der Player jedoch nicht erneut spielen möchte, wird das Programm beendet.

Wenn der Computer die Zahl errät, fällt dem Spieler eine Zahl ein. Der Computer druckt eine Nummer aus und fragt, ob die Nummer des Spielers höher oder niedriger ist. Der Computer macht so lange weiter, bis er die Nummer gefunden hat.

Ich suche Feedback zu absolut allem, was mich zu einem besseren Programmierer machen könnte, insbesondere zu einem besseren C ++ - Programmierer, wie zum Beispiel:

• Optimierung
• Schlechte und gute Praxis
• Codestruktur
• Funktionen und Variablennamen (um ehrlich zu sein, ich bin nicht wirklich gut im Benennen, lol)
• Bugs
• usw

Vielen Dank!

Ich verwende Visual Studio Community 2019 Version 16.7.6

Globals.h

#ifndef GUARD_GLOBALS_H
#define GUARD_GLOBALS_H

static const char COMPUTER_GUESSER = 'c';
static const char PLAYER_GUESSER = 'p';
static const char QUIT = 'q';
static const char ANSWER_IS_YES = 'y';
static const char ANSWER_IS_NO = 'n';
static const int MAX_NUMBER = 100;
static const int MIN_NUMBER = 1;

#endif

BracketingSearch.h

#ifndef GUARD_BRACKETINGSEARCH_H
#define GUARD_BRACKETINGSEARCH_H

int randomNumGenerator(const int max, const int min);
int rangeNumToGuess(const int max, const int min);
int rangeNum(const int max, const int min);

bool startGame();
bool computerOrPlayer(const char userchoice);

bool computerGuesser();
bool playerGuesser();

bool restart();

#endif

BracketingSearch.cpp

#include <iostream>

#include "Globals.h"
#include "BracketingSearch.h"

int randomNumGenerator(const int max, const int min)
{
    return rand() % max + min;
}

int rangeNumToGuess(const int max, const int min)
{
    return ((max - min) / 2) + min;
}

int rangeNum(const int max, const int min)
{
    return max - min;
}

bool startGame()
{
    char userChoice{};

    std::cout <<
        "Who will be the guesser?\n"
        "C - for computer\n"
        "P - for player\n"
        "Q - for quit\n"
        "Type one of the choice: ";
    std::cin >> userChoice;

    computerOrPlayer(tolower(userChoice));
    restart();

    return true;
}

bool computerOrPlayer(const char userchoice)
{
    if (userchoice == COMPUTER_GUESSER)
    {
        return computerGuesser();
    }
    else if (userchoice == PLAYER_GUESSER)
    {
        return playerGuesser();
    }
    else if (userchoice == QUIT)
    {
        std::cout << "Thank you for playing\n";
    }
}

bool computerGuesser()
{
    char userInput{};
    int maxNum = MAX_NUMBER;
    int minNum = MIN_NUMBER;
    int guessNum{};
    int guessCount{ 1 };
    int range;

    std::cout << "Think of a number between 1 to 100\n";

    while(maxNum != minNum)
    {
        ++guessCount;
        range = rangeNum(maxNum, minNum);

        if (range == 1)
        {
            guessNum = maxNum;
        }
        else
        {
            guessNum = rangeNumToGuess(maxNum, minNum);
        }

        std::cout << "Is your number less than: " << guessNum << "?(y/n): ";
        std::cin >> userInput;

        switch (userInput)
        {
        case ANSWER_IS_YES:
            maxNum = guessNum - 1;
            break;
        case ANSWER_IS_NO:
            minNum = guessNum;
            break;
        default:
            std::cout << "That is a wrong option\n";
            guessCount -= 1;
            break;
        }

        if (maxNum == minNum)
        {
            std::cout << "Your number is: " << maxNum << std::endl;
            std::cout << "It took " << guessCount << " guesses for me to guess" << std::endl;
        }

    }
    return true;
}

bool playerGuesser()
{
    int userGuess{};
    int guessCount{ 1 };
    int number = randomNumGenerator(MAX_NUMBER, MIN_NUMBER);

    std::cout << "Enter your guess number: ";

    while (std::cin >> userGuess)
    {
        ++guessCount;

        if (userGuess > number)
        {
            std::cout << "Too high!\n";
        }
        else if (userGuess < number)
        {
            std::cout << "Too low!\n";
        }
        else if (userGuess == number)
        {
            std::cout << 
                "Your guess is correct!\n"
                "It took you: " << guessCount << " guesses\n";
            break;
        }

        std::cout << "Guess another number: ";
    }
    return true;
}

bool restart()
{
    char userChoice{};
    std::cout << "Play again? (y/n): ";
    std::cin >> userChoice;

    char lowerUserChoice = tolower(userChoice);

    if (lowerUserChoice == ANSWER_IS_YES)
    {
        startGame();
    }
    else if (lowerUserChoice == ANSWER_IS_NO)
    {
        computerOrPlayer(QUIT);
    }
    else
    {
        std::cout << "Please choose the available option\n";
        restart();
    }

    return true;
}

main.cpp

#include "BracketingSearch.h"
#include <cstdlib>
#include <ctime>

int main()
{
    srand((unsigned)time(0));

    startGame();

    return 0;
}

Answer 1

Allgemeine Beobachtungen

Um ehrlich zu sein, ist Ihr Code für mich äußerst klar und lesbar. Ich würde nicht vermuten, dass Sie ein Anfänger beim Lesen Ihres Codes waren. Sie haben die Verwendung magischer Zahlen eliminiert und stattdessen globale Konstanten verwendet, was gut ist!

---

Anonyme Namespaces

Das Schlüsselwort bedeutet staticin diesem Zusammenhang, dass es eine interne Verknüpfung hat . Ein anonymer Namespace macht dasselbe, aber sie werden dem Schlüsselwort in C ++ etwas überlegen angesehenstatic .

Der Link, den ich zitiert habe, hat einige großartige Antworten.
Aber hauptsächlich,

• static funktioniert nur für Funktionen und Objekte, ein anonymer Namespace hingegen kann Ihnen eigene Typdefinitionen, Klassen, Strukturen (fast alles) geben ...

// Globals.h

namespace 
{
    // constants
}

Verwenden Sie lieber constexpr

constexpr in C ++

Das Schlüsselwort constexprwurde in C ++ 11 eingeführt und in C ++ 14 verbessert. Es bedeutet ständigen Ausdruck. Ebenso constkann es auf Variablen angewendet werden: Ein Compilerfehler wird ausgelöst, wenn ein Code versucht, den Wert zu ändern. Im Gegensatz zu const, constexprkann auch auf Funktionen und Klassenkonstruktoren angewandt werden. constexpr gibt an, dass der Wert oder Rückgabewert konstant ist und, soweit möglich, zur Kompilierungszeit berechnet wird.

Verwenden constexprSie diese Option, wenn Sie können. Sie teilt dem Compiler mit, dass es sich buchstäblich nur um eine Konstante handelt.
Es zwingt den Compiler, den Wert von etwas zur Kompilierungszeit zu berechnen. Darüber hinaus können Sie es auch als Vorlagenargument übergeben

namespace 
{
    constexpr char COMPUTER_GUESSER { 'c' };
}

---

Benutze ein enum

Dieser Punkt kann von Ihrem Stil abhängen, aber ich denke, dass hier eine Aufzählung erforderlich ist.

Ich spreche über diese Variablen

COMPUTER_GUESSER = 'c';
PLAYER_GUESSER = 'p';
QUIT = 'q';
ANSWER_IS_YES = 'y';
ANSWER_IS_NO = 'n';

Ich glaube, dass es enumSinn macht , ein Hier zu haben, da Sie diese Variablen gruppieren können, da sie alle mit der Wahl des Benutzers zusammenhängen. So würde es aussehen

enum Choice : char 
{
    COMPUTER_GUESSER = 'c',
    PLAYER_GUESSER = 'p',
    QUIT = 'q',
    ANSWER_IS_YES = 'y',
    ANSWER_IS_NO = 'n',
};

if (input == Choice::QUIT) //...

else if (input == Choice::ANSWER_YES) //...

---

Zufällig generieren int

C ++ hat , std::uniform_int_distributionwas besser als C ist rand().

---

Betrachten Sie inliningkleinere Funktionen

int randomNumGenerator(const int max, const int min)
{
    return rand() % max + min;
}

int rangeNumToGuess(const int max, const int min)
{
    return ((max - min) / 2) + min;
}

int rangeNum(const int max, const int min)
{
    return max - min;
}

Das Inlinen dieser Funktionen kann die Leistung erheblich verbessern. Sie müssen jedoch die Definition dieser Funktionen in die Header-Datei einfügen . Sie können angeben, inlineaber es ist wahrscheinlich, dass der Compiler sie selbst inline macht.

Anstatt den CPU-Befehl zum Funktionsaufruf auszuführen, um die Steuerung auf den Funktionskörper zu übertragen, wird eine Kopie des Funktionskörpers ausgeführt, ohne den Aufruf zu generieren.

---

Behandeln Sie immer ungültige Eingaben

std::cout << "Enter your guess number: ";

while (std::cin >> userGuess)
{
    //...
}

Hier std::cinerwartet eine ganze Zahl, wenn der Benutzer versehentlich etwas anderes kommt, std::cinwird fehlschlagen , was zu seltsamem Verhalten in Ihrem Programm

Es gibt einige Möglichkeiten, dieser Artikel ist lesenswert.

Ein kleiner Fehler

In Ihrer restart()Funktion

bool restart()
{
    char userChoice{};
    std::cout << "Play again? (y/n): ";
    std::cin >> userChoice;

    char lowerUserChoice = tolower(userChoice);

    if (lowerUserChoice == ANSWER_IS_YES)
    {
        startGame();
    }
    else if (lowerUserChoice == ANSWER_IS_NO)
    {
        computerOrPlayer(QUIT);
    }
    else
    {
        std::cout << "Please choose the available option\n";
        restart();
    }

    return true;
}

Da Sie rekursiv restart()ungültige Eingaben aufrufen , sollten Sie returnden Wert erhalten, den Sie erhalten. Andernfalls gibt die Funktion nichts zurück

else 
{ 
    std::cout << "Please choose a valid option!\n";
    return restart();
}

Answer 2

Wie bereits erwähnt, ist Ihr Code im Allgemeinen ziemlich gut.

Schalten Sie Warnungen ein und beheben Sie sie.

computerOrPlayersoll ein zurückgeben bool, tut es aber nicht immer.

Leider warnen C ++ - Compiler standardmäßig nicht vor diesem unerwünschten Fehler, können ihn jedoch im Allgemeinen erkennen - wenn Sie die entsprechenden Warnungen aktiviert haben.

Für gcc und clang empfehle ich, der Befehlszeile die folgenden Flags hinzuzufügen : -Werror -Wall -Wextra. Im Detail:

• -Werror: Warnungen als Fehler behandeln.
• -Wall: Aktiviere viele Warnungen (nicht alle, trotz des Namens).
• -Wextra: Aktivieren Sie eine weitere Reihe von Warnungen (immer noch nicht alle).

Weitere Optionen sind die Verwendung von Lintern wie cppcheck.

Compiler-Warnungen und -Linters sind wie automatisierte Prüfer, von unschätzbarem Wert und viel reaktionsschneller als Menschen.

Wofür sind Ihre Rückgabetypen?

Viele Ihrer Funktionen geben a zurück bool, aber oft überprüfen Sie den Rückgabewert Ihrer Funktionsaufrufe nicht.

Sie müssen entscheiden, ob die Funktion wichtige Informationen enthält oder nicht, und sich dann an die Entscheidung halten:

• Wenn dies der Fall ist, sollte ein Wert zurückgegeben und dieser Wert an der Anrufstelle überprüft werden.
• Wenn es nichts zu melden hat, sollte es nichts zurückgeben ( void).

Das [[nodiscard]]Attribut ruft die Hilfe des Compilers auf, um sicherzustellen, dass Sie nicht vergessen, einen Rückgabewert zu überprüfen:

[[nodiscard]] bool yourfunction();

Verwenden Sie Namespaces.

Das Definieren von Symbolen im globalen Namespace ist in C ++ nicht idiomatisch. Der globale Namespace ist bereits ziemlich voll mit allen C-Symbolen, ohne dass das Chaos vergrößert werden muss.

Stattdessen wird empfohlen, dass jedes Projekt einen eigenen Namespace und möglicherweise Sub-Namespaces hat, wenn mehrere Module vorhanden sind - obwohl dies hier übertrieben wäre.

namespace guessing_game {
}

Was ist öffentlich, was ist privat?

Ihr BracketingSearch.hmacht viele Signaturen verfügbar, aber der Client verwendet nur eine .

Ein genau definiertes Modul macht normalerweise nur eine Teilmenge seiner Typen und Funktionen verfügbar - dies ist seine öffentliche Schnittstelle - und der Rest sollte "versteckt" und für den Rest der Welt unzugänglich sein.

In Ihrem Fall können wir sehen, dass mainimmer nur aufgerufen wird startGame: Es scheint, dass dies Ihre öffentliche API ist und alles andere ein Implementierungsdetail ist.

In diesem Fall sollte der BracketingSearch.hHeader nur verfügbar machen startGame: nicht die anderen Funktionen, auch nicht die Konstanten.

Die anderen Funktionen und Konstanten können in privaten Headern deklariert werden , die entweder nur von anderen privaten Headern oder von Quelldateien enthalten sind.

Ein Beispiel für Organisation:

include/
    guessing_game/            <-- matches namespace
        BracketingSearch.h
src/
    guessing_game/
        BracketingSearchImpl.hpp
        BracketingSearchImpl.cpp
        BracketingSearch.cpp

Dann BracketingSearch.cppsieht es so aus:

#include "guessing_game/BracketingSearch.h"
#include "guessing_game/BracketingSearchImpl.h"

namespace guessing_game {

void startGame() {
   ...
}

} // namespace guessing_game

Und BracketingSearchImpl.cppwird aussehen wie:

#include "guessing_game/BracketingSearchImpl.h"

namespace guessing_game {

namespace {
    // ... constants ...
} // anonymous namespace

int randomNumGenerator(const int max, const int min)
{
    return rand() % max + min;
}

int rangeNumToGuess(const int max, const int min)
{
    return ((max - min) / 2) + min;
}

int rangeNum(const int max, const int min)
{
    return max - min;
}

// ... other functions ...

} // namespace guessing_game

Und die Schnittstelle ist klar zu verwenden - sie können nur das verwenden, was im (öffentlichen) Header deklariert ist.

Hinweis: Dieses öffentliche / private Spiel ist rekursiv. Wenn randomNumGeneratores beispielsweise nicht außerhalb verwendet wird BracketingSearchImpl.cpp, sollte es NICHT in deklariert BracketingSearchImpl.hppund in den anonymen Namespace verschoben werden.

Vermeiden Sie globale Variablen

Das Verlassen auf globale Variablen führt zu Problemen beim Testen, Multithreading usw. Es wird am besten vermieden.

In Ihrem Fall verlassen Sie sich auf 3 globale Variablen:

1. Der Zustand von rand().
2. std::cin.
3. std::cout.

In C ++ 11 wurde der <random>Header eingeführt. Dies ist die empfohlene Methode zum Generieren von Zufallszahlen. Dadurch wird vermieden, dass Sie sich auf Folgendes verlassen rand():

• Den Samen weitergeben an startGame.
• Verwenden Sie eine Verteilung aus dem <random>Header.

Für die E / A-Streams gibt es zwei Möglichkeiten:

• Nehmen Sie einfach std::ostream&und std::istream&als Argument startGame.
• Trennen Sie die E / A hinter ihrer eigenen Schnittstelle und übergeben Sie die Schnittstelle an startGame.

Angesichts des geringen Umfangs dieses Spiels; Ich würde empfehlen, nur die Bäche zu passieren.

Hinweis: Wenn Sie mit C ++ besser vertraut sind, sollten Sie sich mit dem Sans IO-Design oder der hexadezimalen Architektur befassen. Die Idee ist, dass E / A an den Rand der Anwendung verschoben werden und alles in der Anwendung nur mit dem Geschäft interagieren sollte. orientierte Schnittstellen. Dies geht auch mit der Abhängigkeitsinjektion einher.

Tests

Sie sollten Ihren Code testen.

Wie geschrieben ist es aufgrund der Verwendung globaler Variablen schwierig zu testen. Sobald sie entfernt sind (siehe vorherigen Punkt), wird es viel einfacher.

Durch Testen können Sie sicherstellen, dass:

• Ungültige Eingaben werden korrekt behandelt.
• Randfälle werden korrekt behandelt.
• ...

Und gibt Ihnen mehr Sicherheit, dass Sie beim Ändern Ihres Codes nicht alles kaputt machen.

Answer 3

Sie haben eine ziemlich schöne Struktur. Und obwohl es für diese Projektgröße ein bisschen viel ist, ist es ein gutes Training für größere Dinge.

Dennoch static constist streng minderwertig, wo constexpreine Wahl ist. Enum-Konstanten sind ebenfalls eine gute Option.

Markierungsparameter constkönnen nützlich sein, um längere Funktionen zu definieren, die Sie lobenswerterweise vermeiden. Aber für Vorwärtsdeklarationen, insbesondere in einer Header-Datei, sind sie nur nutzlose Unordnung, die die Aufmerksamkeit erregt, die anderswo besser investiert ist.

Ihr Sortiment ist neugierig:

1. Sie verwenden ein geschlossenes Intervall. Dies ist in der Programmierung und insbesondere in C ++ selten, da es umständlich und fehleranfällig ist. Geschlossene Zahlenbereiche sind möglicherweise nicht ganz so selten wie Iterator- und Zeigerbereiche, aber das gleiche Prinzip gilt.
   Und wer hätte gedacht, dass Ihre Berechnung für die Größe des Bereichs max - min + 1oft um eins abweicht, was Sie teilweise mit zusätzlichem Code kompensieren.
2. Das Ende vor dem Anfang zu geben, ist sehr unerwartet, nicht nur bei der Programmierung, insbesondere mit C ++, sondern auch für die natürliche Sprache, nicht dass letztere immer eine zuverlässige Anleitung ist.

rand()ist im Allgemeinen ein schreckliches RNG. Was nicht allzu überraschend ist, wenn man bedenkt, dass es für einige antidiluvianische Vorfahren oft abwärtskompatibel ist und die Standardschnittstelle etwas restriktiv ist. Wenn Sie eine bessere mit zuverlässigerer Qualität wünschen, sollten Sie ein Upgrade auf in Betracht ziehen .

randomNumGenerator()ist falsch. maxist nur die Größe des Ausgabebereichs, wenn min1 ist, im Allgemeinen ist es (max - min + 1). Nicht dass diese Methode zur Zuordnung der Zufälligkeit zu dem Intervall, das Sie benötigen, im Allgemeinen nicht zweifelhaft ist. Es gibt auch einen Grund <random>dafür std::uniform_int_distribution.

Ich bin mir nicht sicher, was rangeNum()ich berechnen soll. Wenn es die Größe des Bereichs sein sollte, ist es falsch, siehe oben. Durch das Korrigieren rangeNumToGuess()entfällt auf jeden Fall die Notwendigkeit eines Anrufers, sodass auch dieser beschnitten werden kann.

Ich schlage vor, Funktionsnamen-Aktionen auszuführen: rangeNumGenerator()wird getRandomNumber()und rangeNumGuess()wird guessNumber().

Das Argument zu tolower()darf nicht negativ sein . Und ja, das heißt, du musst besetzen unsigned char.
Ziehen Sie in Betracht, eine neue Funktion zu extrahieren, um eine charvom Benutzer abzurufen und in Kleinbuchstaben umzuwandeln. Sie brauchen es an mindestens zwei Stellen und verwandeln es nur in eine, schlecht. Auf diese Weise können Sie auch eine Variable in beiden Aufrufern entfernen.

Sie könnten auch switchin verwenden computerOrPlayer().

Wenn eine Funktion immer das gleiche Ergebnis zurückgibt, sollten Sie sie zu einer Funktion machen void.

Sie haben uneingeschränkte Rekursion in restart().
Verlassen Sie sich nicht auf den Compiler, um die Tail-Call-Optimierung durchzuführen, insbesondere, weil Sie returndas Ergebnis des rekursiven Aufrufs vergessen haben , um daraus einen Tail-Call zu machen. Zumindest sind keine nicht trivialen Dtoren beteiligt, aber die Fluchtanalyse könnte immer noch zu viel sein, wenn sie überhaupt versucht wird.
Verlassen Sie sich nicht darauf, dass der Benutzer zu ungeduldig ist, um genügend Frames zu sammeln, um einen Stapelüberlauf zu verursachen.

main()hat return 0;am Ende ein implizites . Für was auch immer das wert ist.