2
\$\begingroup\$

I'm new to C++ and while learning, I wrote a simple program that asks for a word, shuffle sthe characters and then asks another person to find the original word, given the shuffled letters.

I would like to know what is right and wrong in this code, what and where can I improve.

#include <iostream>

void shuffle(std::string& word);

int main(int argc, char *argv[])
{
    std::string word, shuffled, guess;
    int count(0);

    std::cout << "Word to guess: ";
    std::cin >> word;

    shuffled = word;
    shuffle(shuffled);

    do {
        std::cout << "Guess(" << shuffled << "): ";
        std::cin >> guess;
        count++;
        if (guess != word) {
            std::cout << "No." << std::endl;
        }
    } while (guess != word);

    std::cout << "Congratulation!" << std::endl;
    std::cout << "Word found in " << count
              << " guess" << (count>1?"es":"")
              << "." << std::endl;

    return 0;
}

void shuffle(std::string& word)
{
    srand(time(nullptr));
    for (unsigned int i(0); i < word.length(); ++i) {
        std::swap(word[i], word[rand() % word.length()]);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ To anyone downvoting and/or voting to close this question, please leave a comment so other users know what's wrong with it and OP can improve the question. \$\endgroup\$ – Mast Apr 17 at 12:58
  • \$\begingroup\$ One thing that could be improved is the title. Please make the title of your question a short description of what your code achieves, not what you want out of a review. Have a look at the help-center for some more information about how to write a good question here. \$\endgroup\$ – Graipher Apr 17 at 13:15
  • 3
    \$\begingroup\$ You don't appear to be including <string>, <ctime> and other relevant headers, so how does this even compile? Please post the full, working code. \$\endgroup\$ – Lundin Apr 17 at 14:15
6
\$\begingroup\$
  1. The most obvious point is probably that the standard library already includes std::shuffle, so there's little point in creating your own.

    • If you do decide to write your own shuffle, you probably want to look up the Fisher-Yates Shuffle algorithm. What you're doing right now can introduce biases (actually, it introduces a couple of different biases in entirely different ways).
  2. If you're going to generate random numbers in C++, you're generally better off using the "new" generators found in <random> instead of using srand/rand from <stdlib.h>/<cstdlib>.

    • Probably also want to use the standard distribution classes instead of rolling your own with the modulus operator (that's one of your current sources of bias1).
  3. Even if you do insist on writing your own and using srand/rand, you must call srand only once, probably when your program starts, not separately each time you're going to use some random numbers.

  4. I advise against using std::endl. Most of the time (including this case) printing a new-line ('\n') will do what you want, and frequently be substantially faster. In addition to writing a new-line, std::endl flushes the stream, which is almost never necessary or desirable.

  5. Instead of:

    std::cout << "Word found in " << count
              << " guess" << (count>1?"es":"")
    

    I'd rather use:

    std::cout << "Word found in " << count
          << (count > 1 ? "guesses" : "guess";
    

    At least to me, this makes it somewhat easier to follow what's going on (and a trade between easier to read and saving half a dozen bytes of string constant seems pretty easy to me, at least most of the time).

  6. Finally, Add the appropriate includes for the features you use.

    • std::string => #include <string>
    • std::srand() => #include <cstdlib>
    • std::time() => #include <ctime>
    • srand() => #include <stdlib.h> // Note the difference between C++ and C version.
    • time() => #include <time.h>

1. consider a generator that produced 0, 1, or 2 and you used %2 to get only 0 or 1. You'd get 1 only when the generator produced 1, but you'd get 0 when it produced either 0 or 2, so assuming the generator produced 0, 1, and 2 equally frequently, your output would be 0 about twice as often as it was 1. With larger ranges, the bias tends to be less extreme (and less visible) but the same basic mechanism causes the same basic problem.

\$\endgroup\$
  • \$\begingroup\$ I like most of this, but I feel that "Just use the standard library" may be good advice when on company time but perhaps misses the point for self-learning projects! \$\endgroup\$ – Josiah Apr 17 at 21:13
  • \$\begingroup\$ I don't quite understand the point 4. What's the difference between \n and std::endl? \$\endgroup\$ – o2640110 Apr 18 at 7:05
  • \$\begingroup\$ For point 6, is there a g++/clang flag to throw warnings when a includes is missing? \$\endgroup\$ – o2640110 Apr 18 at 7:08
  • \$\begingroup\$ In point 2 you said there was some biases introduced with the modulo operator. What are these and why? \$\endgroup\$ – o2640110 Apr 18 at 7:16
  • \$\begingroup\$ Lastly, is there a difference between srand/std::srand and time/std::time? \$\endgroup\$ – o2640110 Apr 18 at 7:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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