Suggestions for code improvement

Question

Good evening, I created a C++ Hangman as an exercise and would like to receive suggestions for code improvement. Thank you from the outset for those who can answer.

Follows the code:

#include <windows.h>
#include <iostream>
#include <string>

std::string palavfr_secreta;
std::string dica;
std::string esconde;

short chances;
short acertos;
short erros;

void jogo(){

    std::string letra_digitada;
    std::string letra_secreta;
    std::string letra_esconde;

    short contador=0;

    std::cout << "================== " << std::endl;
    std::cout << "Dica-------------> " << dica << std::endl;
    std::cout << "Palavra secreta--> " << esconde << std::endl;
    std::cout << "Chances----------> " << chances << std::endl;
    std::cout << "================== " << std::endl << std::endl;
    std::cout << "Digite uma letra-> ";
    std::cin >> letra_digitada;

    std::cin.ignore();

    if(letra_digitada.size()>1){

        system("cls");

        std::cout << "DIGITE SOMENTE UMA LETRA!" << std::endl;

        jogo();

    }else{

        for(unsigned i=0; i<palavfr_secreta.size(); i++){

            letra_secreta=palavfr_secreta.at(i);
            letra_esconde=esconde.at(i);

            if(letra_digitada.compare(letra_esconde)==0){

                system("cls");

                std::cout << "Você não pode digitar a mesma letra mais de uma vez" << std::endl;

                chances--;

                break;

            }else if(letra_digitada.compare(letra_secreta)==0){

                system("cls");

                esconde.at(i)=letra_digitada.at(0);

                contador++;
                acertos++;

            }else if(i==palavfr_secreta.size()-1 && contador==0){

                system("cls");

                std::cout << "Letra não correspondente" << std::endl;

                chances--;
                erros++;
            }
        }
    }

    if(chances==0){

        system("cls");

        std::cout << "======================================================================" << std::endl;
        std::cout << "Você perdeu! A frase ou palavra secreta era " << "'" << palavfr_secreta << "'" << std::endl;
        std::cout << "Total de acertos -> " << acertos << std::endl;
        std::cout << "Total de erros -> " << erros << std::endl;
        std::cout << "======================================================================" << std::endl;

    }else if(esconde.compare(palavfr_secreta)==0){

        system("cls");

        std::cout << "=======================================================================" << std::endl;
        std::cout << "Você acertou! A frase ou palavra secreta era " << "'" << palavfr_secreta << "'" << std::endl;
        std::cout << "Total de acertos -> " << acertos << std::endl;
        std::cout << "Total de erros -> " << erros << std::endl;
        std::cout << "=======================================================================" << std::endl;

    }else{

        jogo();
    }
}

int main(){

    std::cout << "===============Jogo da Forca===============" << std::endl;
    std::cout << "Digite uma palavra ou frase secreta-> ";
    getline(std::cin, palavfr_secreta);

    system("cls");

    std::cout << "Escreva uma dica-> ";
    getline(std::cin, dica);

    system("cls");

    std::cout << "Defina o número de Chances-> ";
    std::cin >> chances;

    for(unsigned i=0; i<palavfr_secreta.size(); i++){

        if(palavfr_secreta.at(i)==' '){
            esconde+=" ";
        }else{
            esconde+="-";
        }
    }

    system("cls");

    jogo();

    return 0;
}

Answer 1

Our format is not quite code review, this applies more to what exists in the English version of Code Review, Still, here are some of my improvement tips that you can make:

• Avoid looping/looping/looping at the expense of recursion. Something you are doing here:

  void jogo(){
      if(chances==0){
          ...
      }else if(esconde.compare(palavfr_secreta)==0){
          ...
      }else{
          jogo();
      }
  

  Not only is it a lot less efficient, it might even blow up the show with the classic Stack Overflow Exception. This is because it is constantly opening up new equal functions in the stack until potentially it bursts.

  The way to solve this problem is by transforming recursion into a loop with while or for for example.

• When you access a letter of a string access by the indexing operator is more idiomatic, the [] instead of palavfr_secreta.at(i)

• Try to bring the variables as close to your usage as possible:

  void jogo(){
  
      std::string letra_digitada;
      std::string letra_secreta;
      std::string letra_esconde; //<----------- declarada aqui
  
      short contador=0;
  
      std::cout << "================== " << std::endl;
      std::cout << "Dica-------------> " << dica << std::endl;
      std::cout << "Palavra secreta--> " << esconde << std::endl;
      std::cout << "Chances----------> " << chances << std::endl;
      std::cout << "================== " << std::endl << std::endl;
      std::cout << "Digite uma letra-> ";
      std::cin >> letra_digitada;
  
      std::cin.ignore();
  
      if(letra_digitada.size()>1){
  
          system("cls");
  
          std::cout << "DIGITE SOMENTE UMA LETRA!" << std::endl;
  
          jogo();
  
      }else{
  
          for(unsigned i=0; i<palavfr_secreta.size(); i++){
  
              letra_secreta=palavfr_secreta.at(i);
              letra_esconde=esconde.at(i); //<---------utilizada aqui
  

  Although it can be (potentially) more efficient in some cases it ends up considerably impairing the reading, as it is difficult to know where the variable is being used. And if it’s from an optimization perspective, it’s probably premature optimization that even the compiler itself can do.

• To know if a certain character exists in a string has the method find of string which facilitates your work by converting your for and first if:

  for(unsigned i=0; i<palavfr_secreta.size(); i++){
      letra_secreta=palavfr_secreta.at(i);
      letra_esconde=esconde.at(i);
  
      if(letra_digitada.compare(letra_esconde)==0){
          ....
  

  In

  if (esconde.find(letra_digitada) != std::string::npos){
  

  Assuming letra_digitada is a char.

• Try to use the most appropriate types for your values. When reading one char as in the case of letra_digitada, read for a type variable char. This makes it easier to compare.

  Can transform this:

  }else if(letra_digitada.compare(letra_secreta)==0){
  

  In this

  }else if(letra_digitada == palavra_secreta[i]){
  

• The block you have for when you can’t find the letter typed in for:

  for(unsigned i=0; i<palavfr_secreta.size(); i++){
      ...
      }else if(i==palavfr_secreta.size()-1 && contador==0){
          system("cls");
          std::cout << "Letra não correspondente" << std::endl;
          chances--;
          erros++;
      }
  }
  

  I should actually be out of the for. I would do that by putting inside the for only the if when the letter exists and changing the value of a flag. Then after the for forehead the flag to know hit some letter.

Applying all these improvements that I suggested the code would look like this:

#include <windows.h>
#include <iostream>
#include <string>

std::string palavra_secreta,dica,esconde;    
short chances,acertos,erros;

void jogo(){
    //nova condição de fim do jogo
    while (acertos < palavra_secreta.size() && chances > 0){ 
        system("cls"); //apenas limpa aqui e não dentro do for como tinha
        std::cout << "================== " << std::endl;
        std::cout << "Dica-------------> " << dica << std::endl;
        std::cout << "Palavra secreta--> " << esconde << std::endl;
        std::cout << "Chances----------> " << chances << std::endl;
        std::cout << "================== " << std::endl << std::endl;
        std::cout << "Digite uma letra-> ";

        char letra_digitada; //agora como char
        std::cin >> letra_digitada;

        if (esconde.find(letra_digitada) != std::string::npos){//testa se existe com find
            system("cls");
            std::cout << "Você não pode digitar a mesma letra mais de uma vez" << std::endl;
            chances--;
        }
        else {
            int novos_acertos = 0;

            for(unsigned int i=0; i < palavra_secreta.size(); i++){
                if(letra_digitada == palavra_secreta[i]){ //apenas atualiza as letras
                    esconde[i]=letra_digitada;
                    novos_acertos++;
                    acertos++;
                }
            }

            if (novos_acertos == 0){ //se não acertou nenhuma mostra mensagem
                system("cls");
                std::cout << "Letra não correspondente" << std::endl;
                chances--;
                erros++;
            }
        }

        if(chances == 0){
            system("cls");
            std::cout << "======================================================================" << std::endl;
            std::cout << "Você perdeu! A frase ou palavra secreta era " << "'" << palavra_secreta << "'" << std::endl;
            std::cout << "Total de acertos -> " << acertos << std::endl;
            std::cout << "Total de erros -> " << erros << std::endl;
            std::cout << "======================================================================" << std::endl;
        }else if(esconde.compare(palavra_secreta) == 0){
            system("cls");
            std::cout << "=======================================================================" << std::endl;
            std::cout << "Você acertou! A frase ou palavra secreta era " << "'" << palavra_secreta << "'" << std::endl;
            std::cout << "Total de acertos -> " << acertos << std::endl;
            std::cout << "Total de erros -> " << erros << std::endl;
            std::cout << "=======================================================================" << std::endl;
        }
    }
}

int main(){
    std::cout << "===============Jogo da Forca===============" << std::endl;
    std::cout << "Digite uma palavra ou frase secreta-> ";
    getline(std::cin, palavra_secreta);

    system("cls");
    std::cout << "Escreva uma dica-> ";
    getline(std::cin, dica);

    system("cls");
    std::cout << "Defina o número de Chances-> ";
    std::cin >> chances;

    for(unsigned i=0; i<palavra_secreta.size(); i++){
        esconde += palavra_secreta[i]==' ' ? " ":"_"; //com ternário para ficar simples
    }

    jogo();
    return 0;
}

Answer 2

I realized that for one person to play another has to type the word and give the hint, so you could save several words per category in a file and pick them randomly.
example in your menu would have something like choose a theme.
-animals
-fruit
-colors etc...
then the player would choose animals and his program would draw a word from the animals file. if you want to look at the functions: Rand() and srand() for randomness.
and the library: fstream to open files.