Оцените С++ код пожалуйста?

Question

[RADION NAZMIEV]

//Напишите программу реализующую игру «Угадай число».Компьютер загадывает число от 0 до 999 (используйте генерацию случайных чисел), 
//а пользователь угадывает его.На каждом шаге угадывающий делает предположение, а задумавший число — сообщает, сколько цифр из числа угаданы 
//и сколько из угаданных цифр занимают правильные позиции в числе.Например, если задумано число 725 и выдвинуто предположение, 
//что задумано число 523, то угаданы две цифры(5 и 2) и одна из них занимает верную позицию.

#include <iostream>
#include <ctime>
#include <string>
#include <random>
#include <algorithm>
#include <iomanip> // для ограничения количества вводимых симолов для std::cin

void startTheGame();
int getUsersGuess();
int getRandomNumber(int min, int max);
void compUsrWthCmptr(std::string userInput, std::string computerInput);

int main()
{
    setlocale(LC_ALL, "Russian");
    srand(static_cast<unsigned int>(time(0)));
    startTheGame();
    return 0;
}

void compUsrWthCmptr(std::string userInput, std::string computerInput)
{
    //  Считаем правильно угаданные позиции

    std::reverse(userInput.begin(), userInput.end());
    std::reverse(computerInput.begin(), computerInput.end());
    int guessedPositions{ 0 };
    for (int i = 0; (i < userInput.length()) && (i < computerInput.length()); ++i)
    {
        if (userInput[i] == computerInput[i])
        {
            guessedPositions++;
        }
    }

    std::string::iterator it_userInput;
    std::string::iterator it_computerInput;

    // Удаляем повторяющиеся цифры

    it_userInput = std::unique(userInput.begin(), userInput.end());
    userInput.resize(std::distance(userInput.begin(), it_userInput));
    it_computerInput = std::unique(computerInput.begin(), computerInput.end());
    computerInput.resize(std::distance(computerInput.begin(), it_computerInput));

    //  Считаем количество правильно угаданных цифр без учета повторяющихся

    int guessedDigits{ 0 };
    for (int i = 0; i < userInput.length(); ++i)
    {
        for (int x = 0; x < computerInput.length(); ++x)
        {
            if (userInput[i] == computerInput[x])
            {
                guessedDigits++;
            }
        }
    }
    std::cout << "  Угадано: " << guessedDigits << ". Соответствует своим разрядам: " << guessedPositions << std::endl << std::endl;

};
void startTheGame()
{
    int pcsRandomNumber = getRandomNumber(0, 999);      //Загаданое число.
    std::cout << "  Компьютер загадал трехзначное число от 0 до 999!\n" << "  Это: " << pcsRandomNumber << std::endl << std::endl;
    std::string pcNumber{ std::to_string(pcsRandomNumber) };
    bool win = false;
    do
    {
        int usersGuess = getUsersGuess();
        std::string guess{ std::to_string(usersGuess) };
        std::cout << "  Ваш вариант : " << guess << std::endl;
        compUsrWthCmptr(guess, pcNumber);
        if (usersGuess == pcsRandomNumber)
        {
            win = true;
            std::cout << "  *** Вы угадали число " << pcsRandomNumber << "!***\n";
        }
    } while (!win);
};

int getUsersGuess()
{
    while (true) // цикл продолжается до тех пор, пока пользователь не введет корректное значение
    {
        std::cout << "  Введите коректное значение: ";
        int a;
        std::cin >> std::setw(3) >> a;

        // Проверка на предыдущее извлечение
        if (std::cin.fail()) // если предыдущее извлечение оказалось неудачным,
        {
            std::cin.clear(); // то возвращаем cin в 'обычный' режим работы
            std::cin.ignore(32767, '\n'); // и удаляем значения предыдущего ввода из входного буфера
            std::cout << "  Предыдущее извлечение оказалось неудачным. Попытайтесь еще раз.\n\n";
        }
        else
        {
            if (a >= 1000 || a < 0)
            {
                std::cin.ignore(32767, '\n'); // удаляем лишние значения
                std::cout << "  Введенное число вне требуемого диапазонате. Попытайтесь еще раз.\n\n";
            }
            else
            {
                std::cin.ignore(32767, '\n'); // удаляем лишние значения
                return a;
            }
        }
    }
}
int getRandomNumber(int min, int max)
{
    return static_cast<int>(rand() % (max - min + 1) + min);
}

Comments

[tukreb]

нормально.

Answer 1

[Adamos]

Если вы собираетесь заниматься Крестами более серьезно, чем эта лаба в С-стиле, обратите внимание на следующее. Ваши функции выполняют всякие довольно банальные операции, но при этом называются compUsrWthCmptr и предполагают исполнение именно этих, нужных сейчас, действий. А точнее - понимание того, что и зачем делает эта функция, при работе с ней.
Для будущего попробуйте абстрагироваться от задачи и писать функции, выполняющие именно те действия, которые они выполняют, без всякого знания о том, что происходит вне их. Сравнивает у вас функция две строки - вот и пускай сравнивает. Ей надо знать только алгоритм сравнения, а не то, что этот инпут - пользовательский. В нее приходит две строки, выходит результат - и все.
А вот в вызывающей ее функции вы напишете более высокоуровневый код подготовки ввода к обработке этой функцией и действия по ее результату. И эта функция, в отличие от вашей, будет читаться легко и непринужденно.
Так у вас низкоуровневое копание в байтах будет изолировано от более человеческой логики программы, и разделив код на подобные уровни - значительно легче с ним работать. Особенно когда вы перейдете от процедурщины к классам.

Comments

[Евгений Шатунов]

Все верно.
Это - декомпозиция. Важное умение не только для изложения кода, но и для изложения своих мыслей.

Answer 2

[User700]

Зачем разворачивать строчки, если можно запустить цикл сравнения в обратную сторону.
Как и сказали, странно видеть вывод в cout в "утилитарных" функциях. Логично было бы возвращать эти данные функцией (в кортеже/паре или своей структуре).
Скорее всего алгоритмическая ошибка: если загадано 1223, а пользователь ввел 92227, то он угадал одну или две цифры? Наверно две.
Если бы функция только смотрела на строки, то передавать надо было их по конст. ссылке или string_view. Т.к. меняет, то да, слелано правильно -- по значению. Здесь объекты -- короткие строки, не влияет; а в общем случае тогда при вызове можно передавать те данные, которые дальше не нужны:
compUsrWthCmptr(std::move(guess), pcNumber);
Или проще убрать объявление guess, и записать

compUsrWthCmptr(std::to_string(usersGuess), pcNumber);

Странное название startTheGame. Как будто есть еще асинхронные pause и stop. Наверно просто game подошло лучше.