1
\$\begingroup\$

I wonder whether I included too many libraries; also can you point out obvious beginner mistakes?

Is the while loop the best for what I intend, or is there a better way?

Also, is it possible to put more into functions such that the main() looks like this:

int main()

{

call function1

call function2

etc.

return 0;

}
#include <iostream>
#include <fstream>
#include <stdlib.h>
#include <time.h>
#include <string>
#include <vector>
#include <cstring>
using namespace std;

string random_line()
{
    string line;
    vector<string> lines;
    srand(time(0));
    //input file stream
    ifstream file("wordlist.txt");
    //count number of total lines in the file and store the lines in the string vector
    int total_lines=0;
    while(getline(file,line))
    {
        total_lines++;
        lines.push_back(line);
    }
    //generate a random number between 0 and count of total lines
    int random_number=rand()%total_lines;
    //fetch the line where line index (starting from 0) matches with the random number
    return lines[random_number];
}

int main()
{
    string secret_word=random_line();
    cout << secret_word; //this has to be cut at the end
    int word_length=secret_word.length();
    string guess_word="";
    string guessed_letters="";
    for (int i=0; i<word_length; ++i)
    {
        guess_word.append("_");
    }
    int remaining_fails=10;
    string guess="";

    while(remaining_fails!=0)
    {
        if(guess_word==secret_word)
        {
            cout << "Congratulations, the word is: "<<secret_word<<endl;
            return 0;
        }
        else
        {
            cout << string(50, '\n');
            cout << guess_word << endl;
            cout << "Remaining fails: " << remaining_fails << endl;
            cout << "Guessed letters: " << guessed_letters << endl;
            cout << "Guess your letter!" << endl;
            cin  >> guess;
            if(guess.length() !=1)
            {
                cout << "Please only enter one character!";
            }
            else if(strstr(secret_word.c_str(),guess.c_str()))
            {
                for(int i=0; i<word_length; ++i)
                {
                    if (guess.at(0)==secret_word.at(i))
                    {
                        guess_word.at(i)=guess.at(0);
                    }
                    else
                    {

                    }
                }
            }
            else
            {
                guessed_letters.append(guess);
                remaining_fails--;
            }
        }


    }
    cout << "You loose!"<<endl;
    return 0;
}

The wordlist.txt contains?

word
trio
list
\$\endgroup\$
2
  • \$\begingroup\$ What you mean too many includes? Are you using symbols from each of them? If yes then no, if no then yes. \$\endgroup\$
    – slepic
    Feb 12, 2020 at 6:21
  • \$\begingroup\$ @slepic ah ok, i do use symbols from each of them. \$\endgroup\$
    – SAJW
    Feb 12, 2020 at 6:32

2 Answers 2

5
\$\begingroup\$
  1. I'm not sure if I'd use the phrase "too many", but I think you've included a header or two you'd probably be better off without.

    The most obvious example would be <cstring>. There's rarely a good reason to use any of the <cstring> functions in C++ code (and I don't see any hint that this code is one of the rare exceptions either).

    Right now, the main (only?) thing I see you using from <cstring> is strstr. std::string has a find member that can find a substring like strstr does.

  2. Your code also seems somewhat inconsistent, at least to me. For example, in one place you create a string with N repetitions of a single character using code like: std::string(character, repetitions). In another place you do roughly the same thing using a loop to append on underscore at a time. Since they're doing the same thing, it would be better for them to do it the same way (preferably the constructor for std::string).

  3. Right now you use a string to hold a single character (and enforce its being only one character). Since it's only a single char, I'd rather just use a char to hold it.

  4. As a general rule of thumb, I'd prefer a well-chosen algorithm from the standard library over a raw loop.

  5. Yes, you can (and in my opinion, should) break the code up into more functions (and based on your question, it seems like you already agree, but may not be sure how to do it).

  6. I'd generally prefer to use the new random-number generation routines in <random> over the srand/rand from the C library. Likewise, I'd rather use the distribution classes in the standard library over the rand() % total_lines you have now. (but in fairness, they are more work to use, and it's unlikely to make a difference anybody's likely to notice or care about (but in many other cases, it's more important).

  7. I'd avoid using namespace std; I know at first it seems painful to type in std:: everywhere, but it's still better than using namespace std;.

  8. There are also neater ways to read words from a file into an array of strings, such as:

    std::ifstream infile("wordlist.txt");
    
    std::vector<std::string>{std::istream_iterator<std::string>(infile), {}};
    

    One point though: this will produce different results if your input file has multiple words on one line. In this case your code treats a whole line as one word, but this treats each word separately.

  9. I generally advise against using std::endl. It's probably harmless in this case, but a habit I'd try to avoid anyway.

\$\endgroup\$
4
  • \$\begingroup\$ 2. ah, nice catch, to be honest i put some of the code together like lego. What you mean with "ctor"? i googled it but i'm not quite sure, has it something with contructors to do? 3. I thought you can do this operation only with the same types, thanks for pointing that out. 4.does a good written program still have variables initialized in the main() or can that also be outsourced? 7. would an alternative be to only write "using std::cout" to save a few "std::"s? 8. that's huge, because most lists don't come in the format i need(ed) 9. is std::endl always worse than "\n"? \$\endgroup\$
    – SAJW
    Feb 12, 2020 at 7:29
  • \$\begingroup\$ @SAJW: Sorry. "ctor" is just a common shorthand for "constructor". Can certainly initialize things in main if it makes sense (e.g., things shared between the other functions). Use the smallest scope that works. Writing using std::cout is certainly preferable to using namespace std;anyway. Like I said, I've taken to advising against std::endl in general--even when you want exactly what it does, it's been so heavily abused by so many for so long that nobody will really know whether you wanted what it does, or just used it because everybody thinks they should. \$\endgroup\$ Feb 12, 2020 at 7:37
  • \$\begingroup\$ so to avoid confusion wether i want what std::endl does i should be using std::flush instead (with "\n")? (if i want what endl does) \$\endgroup\$
    – SAJW
    Feb 12, 2020 at 8:01
  • \$\begingroup\$ @SAJW: Yes. Usually you just want stream << '\n', but if you really want to flush as well, stream << '\n' << std::flush; (with the proviso that this is my opinion, and others undoubtedly disagree). \$\endgroup\$ Feb 12, 2020 at 15:38
3
\$\begingroup\$

I have some observations not mentioned in Jerry Coffin's answer.


Prefer <cstdlib> over <stdlib.h>. The former puts its identifiers into the std namespace where we want them.


To select one line at random from all the input lines, we don't need to store them all. There's a single-pass algorithm that selects each line with equal probability, that's quite simple to understand:

  • Choose the first line with probability 1 (i.e. always).
  • Read the second line, and with probability ½ choose it instead of the first.
  • Read the third line, and with probability ⅓ choose it instead of one of the first two.
  • Read the fourth line, and replace selected line with probability ¼.
  • and so on...

In code, that looks like this:

#include <istream>
#include <random>
#include <string>

#include <utility>

std::string random_line(std::istream& input)
{
    static auto gen = std::mt19937{std::random_device{}()};
    auto count = 0u;
    auto selected = std::string{};
    std::string line;
    while (std::getline(input, line)) {
        if (std::uniform_int_distribution{0u,count++}(gen) == 0) {
            selected = std::move(line);
        }
    }
    return selected;
}

We can demonstrate it with a simple test program, that will pick one of six lines, approximately 20000 times each:

#include <iostream>
#include <map>
#include <sstream>
int main()
{
    auto const lines = "one\n"
                       "two\n"
                       "three\n"
                       "four\n"
                       "five\n"
                       "six\n";
    auto counts = std::map<std::string,unsigned int>{};
    for (auto i = 0;  i < 120000;  ++i) {
        auto is = std::istringstream{lines};
        ++counts[random_line(is)];
    }

    {
        // print the counts
        auto is = std::istringstream{lines};
        std::string line;
        while (std::getline(is, line)) {
            std::cout << line << ": " << counts[line] << '\n';
        }
    }
}

Always check whether reading input succeeded:

        std::cin >> guess;

If we reach the end of input, then guess will be empty (or in C++03 and earlier, won't be written to), and we'll enter an infinite loop.

Similarly, make sure that file is good after opening it in random_line().

Consider ignoring non-word characters (e.g. !, @, ...) as invalid input, unless your word list actually contains them. And consider being nice to the user if they enter a guess they've already tried - don't penalise that as a wrong guess.


Minor: Spelling error in the final result:

std::cout << "You lose!\n";
\$\endgroup\$
8
  • \$\begingroup\$ Hi, with your example the wordlist would be limited to a magic number(here: 120000) I specify in the code or? Thanks for your answer! \$\endgroup\$
    – SAJW
    Feb 14, 2020 at 12:52
  • \$\begingroup\$ No, that magic number is just for the test program - it determines how many times we randomly select a line, before showing how many times each was selected. There's no limit to the number of entries in the wordlist, other than the maximum file size your platform supports. \$\endgroup\$ Feb 14, 2020 at 13:05
  • \$\begingroup\$ Hi, i tried to compile this but it says "error: expected ')' before '{' token" in the random_line function right at the if statement. Could you please fix it? Here is the pastebin: pastebin.com/L9Wz6ubk \$\endgroup\$
    – SAJW
    Feb 20, 2020 at 20:58
  • \$\begingroup\$ oh, i had gcc version 8.2, i will try with the newest! thanks for pointing that out. \$\endgroup\$
    – SAJW
    Feb 21, 2020 at 12:26
  • \$\begingroup\$ It's the C++ version rather than GCC version that's important: I used g++ -std=c++17 -Wall -Wextra (GCC version 9.2), but it won't build with -std=c++14. \$\endgroup\$ Feb 21, 2020 at 12:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.