Code Analysis/Critique

[MrCluckyToYou]

Hello,

First off I want to say that I have been coding on and off as a hobby, for a long time. I started with BASIC when I was a lot younger and I started learning C++ several years ago only to put it down for years. Now I'm trying again and my ultimate goal right now is to be able to make a video game. I want to start with clones of my favorite games and then move on to designing and building my own game. As such, I have subscribed to several different sources for learning C/C++, BASIC, C#, python and others but I'm trying to focus on one thing at a time and for now, that one thing is C++. I have found that for me, a book is better than a video for me to follow along and be able to produce something quickly, even if it's just small bits at a time. So I'm working my way through a book called Beginning C++ Through Game Programming, Third Edition (I am using resources that are either free or very cheap, aka sometimes outdated). Anyway, in this book I have reached chapter 5 and at the end of the chapter he asks us to re-write the hangman program from chapter 4 using functions. The code below is my solution to the problem, and I'm just wanting people with experience to look it over and tell me how I did and offer advice and feedback, since I'm doing this on my own and lack the benefit of an experienced instructor. I appreciate your time and consideration.

EDIT: I should mention that he specifically asked us to write two functions, one to get the player's guess and one to check it against the word to be guessed. The majority of the code was his, with the function design and implementation being my solution.

//============================================================================
// Name        : Hangman.cpp
// Author      : MrCluckyToYou
// Version     :
// Copyright   :
// Description : The classic game of Hangman
//============================================================================

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>
#include <ctime>
#include <cctype>

using namespace std;

// function declarations.
char getGuess(string used);
bool chkGuess(char guess, const string THE_WORD);

int main()
{
	//setup
	const int MAX_WRONG = 8; // maximum number of incorrect guesses allowed.

	vector<string> words; // collection of possible words to guess.
	words.push_back("GUESS");
	words.push_back("HANGMAN");
	words.push_back("DIFFICULT");
	words.push_back("CLUCKY");

	// choose a random word to guess from the list.
	srand(static_cast<unsigned int>(time(0)));
	random_shuffle(words.begin(), words.end());

	const string THE_WORD = words[0];			// word to guess.
	int wrong = 0;							// number of incorrect guesses.
	char guess;							// letter guessed.
	string soFar(THE_WORD.size(), '-');			// word guessed so far.
	string used = "";					        // letters already guessed.

	cout << "Welcome to Hangman. Good luck!\n";

	// main loop
	while (wrong < MAX_WRONG && soFar != THE_WORD)
	{
		guess = getGuess(used);

		if (chkGuess(guess, THE_WORD))
		{
			for (unsigned int i = 0; i < THE_WORD.length(); ++i)
			{
				if (THE_WORD[i] == guess)
				{
					soFar[i] = guess;
				}
			}
		}
		else
		{
			++wrong;
		}

		used += guess;
	}

	// shut down
	if (wrong == MAX_WRONG)
	{
		cout << "\nYou've been hanged!";
	}
	else
	{
		cout << "\nYou guessed it!";
	}

	cout << "\nThe word was " << THE_WORD << endl;

	return 0;
}

// function to get the player's guess, providing it hasn't already been guessed.
char getGuess(string used)
{
	char guess;
	cout << "\n\nEnter your guess: ";
	cin >> guess;
	guess = toupper(guess);

	while (used.find(guess) != string::npos)
	{
		cout << "\nYou've already guessed " << guess << ".\n";
		cout << "Enter your guess: ";
		cin >> guess;
		guess = toupper(guess);
	}

	return guess;
}

// function to check the player's guess against the word to guess.
bool chkGuess(char guess, const string THE_WORD)
{
	if (THE_WORD.find(guess) != string::npos)
	{
		cout << "That's right! " << guess << " is in the word.\n";
		return true;
	}
	else
	{
		cout << "Sorry, " << guess << " isn't in the word.\n";
	}

	return false;
}

[Grime]

Well organized. Some people will say that you should have even lesser code in main function but I can't critique on that.

You don't need to use random_shuffle you can just pick a random index from the vector since the only reason you're shuffling is to pick a random choice. random_shuffle is just not required.

eg: const string THE_WORD = words[rand() % words.size()]

Then again if you have a long list of words you wouldn't store it in a vector but rather in chunks of files. You don't have to worry about that for now.

You forgot an output for when the user guessed a wrong letter. That's all you're missing.

[E0p9LyTq]

The biggest thing that stands out for me as being somewhat inefficient is shuffling the container of words.

Just randomly pick one of the elements.

[jlb]

It also doesn't make much sense to const qualify a parameter sent by value. And note that passing std::string by value can be an expensive operation and you should consider passing them by reference/const reference.

[dutch]

random_shuffle doesn't even exist in the language anymore (deprecated C++14, removed C++17).

Although shuffling isn't a bad idea since if the user wants to play again you'll want a different word, so you could just go through the shuffled list. But you should probably read the words in from a (possibly encrypted) file.

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <algorithm>
#include <random>
#include <chrono>
#include <cstdlib>

void read_words(std::vector<std::string>& word_list)
{
    unsigned seed = std::chrono::system_clock::
                    now().time_since_epoch().count();
    auto rng = std::mt19937(seed);
    std::ifstream fin("words");
    if (!fin)
    {
        std::cerr << "The word list is missing.\n";
        std::exit(EXIT_FAILURE);
    }
    for (std::string word; fin >> word; )
        word_list.push_back(word);
    shuffle(word_list.begin(), word_list.end(), rng);
}

int main() {    
    std::vector<std::string> word_list;
    read_words(word_list);
    for (const auto& word: word_list)
        std::cout << word << '\n';
}

But maybe it's not a good idea to read all the words in since ultimately you would want a large number of words in the word file. So it would probably be best to randomly select words directly from the file, perhaps in the order that they are in the file with an index value stored at the start of the file that is incremented every time a word is read.

Here's an example of that. It reads the next word in the list each time it is run, wrapping back around to the first one when it reaches the end.

#include <iostream>
#include <iomanip>
#include <fstream>
#include <string>
#include <vector>
#include <cstdlib>

std::string read_word()
{
    std::fstream f("words");
    if (!f)
    {
        std::cerr << "The word list is missing.\n";
        std::exit(EXIT_FAILURE);
    }
    int n;
    f >> n;
    ++n;
    std::string word;
    for (int i = 0; i < n; ++i)
        if (!(f >> word))
        {
            f.clear();
            f.seekg(0, f.beg);
            f >> n;
            n = 1;
            f >> word;
            break;
        }
    f.seekp(0, f.beg);
    f << std::setfill('0') << std::setw(5) << n;
    return word;
}

int main() {
    std::string word = read_word();
    std::cout << word << '\n';
}

The number at the beginning of the file needs to take up 5 positions. So the initial value would be a 0 preceded by either 4 spaces or 4 zeroes (the zeroes show up visually so that's what I used).

00000
alpha
beta
gamma
delta
epsilon
zeta
eta
theta
iota
kappa
lambda

It would probably be better to just make it a binary file with the counter stored at the beginning and all the words stored in a fixed amount of space (the size of the largest word or bigger). Then you could jump directly to the current word.

[Grime]

dutch can I ask if there is a specific reason why you're fitting the increment number (the first line of the file) to 5 characters?

One thought, suppose you're in the 1000th word. Then your program would have to extract 999 words before getting the 1000th word. Is there no better way?

Also, in the second approach there's one flaw that every time you run the program you get the same list of words. So you would have to add randomness.

Before the incremented n value at the beginning of the file, you could keep another sequence of digits which would represent the number of words in the file.

Then you could do something like this:

++n;
random_number = rand() % max_words;
n += rand; 
while(n > max_words)
   n -= max_words;

(just an example, I know everybody hates rand())

One thought, suppose you're in the 1000th word. Then your program would have to extract 999 words before getting the 1000th word. Is there no better way?

And because it would become even more expensive to extract words if the extraction were to be randomized (as random_number can be as large as max_words) I was thinking something like this:

The words are arranged in ascending order of number of letters. And at the beginning of the file, the number of words of specific number of characters is mentioned.

Something like this:

21 // marks characters for pre-info including itself
00000 // increment variable, necessary to avoid repetition
0 // no of 1 letter words
0 // no of 2 letter words
4 // no of 3 letter words
25 // no of 6 letter words
END // marks end of pre-info
beta
thet
iota // 3 letter words beginning
// etc. 

And using this information we can position the file pointer accordingly.

For example if n were 2, we need to get the first word.
we skip 21 characters + 3x1 = 24 characters and voila we're at the second word.

For the randomizer we can use something other than just addition, but a function that never lets two inputs have the same output.

I ran out of time, I'll edit later and make it more neat with examples.

Anybody got better ideas?

[dhayden]

Be a little more descriptive when you comment your functions. It usually helps to describe each parameter and the return value. I'd put the same comment above the declarations of the functions as well as the definitions. That way when you look at the code 3 months from now, you just have to look at the declarations to find out the functions do.

Great job commenting your variables.

guess = getGuess(used);
s
e
v
e
n
t
e
e
n

l
i
n
e
s

l
a
t
e
r
used += guess;

Keep related code together.

Getting and validating input is a perfect example of a loop where the exit condition is really in the middle. Don't be afraid to code it that way.

char getGuess(string used)
{
	char guess;
	while (true) {
		cout << "\n\nEnter your guess: ";
		cin >> guess;
		guess = toupper(guess);
		if (used.find(guess) == string::npos) {
			return guess;
		}

		cout << "\nYou've already guessed " << guess << ".\n";
	}
}

	if (wrong == MAX_WRONG)
	{
		cout << "\nYou've been hanged!";
	}
	else
	{
		cout << "\nYou guessed it!";
	}

That's a whole lot of lines for not much meaning. My philosophy is that the braces are there for the compiler and the humans discern the block structure from the indentation. Therefore I like to keep the braces inline with the code. It means that you don't have to scroll vertically all the time to see what's happening. This is mostly a matter of personal taste and people are passionate about it, so feel free to ignore me here. :)

	if (wrong == MAX_WRONG) {
		cout << "\nYou've been hanged!";
	} else {
		cout << "\nYou guessed it!";
	}

[MrCluckyToYou]

Wow, thanks everyone! I have to admit I was a bit overwhelmed and even discouraged when I glanced at the post last night (I work 3rd shift and we're not supposed to be on our phones, but I had to KNOW). After reading the first couple replies I felt good, but immediately after I skimmed through dutch's reply I felt I was in way over my head and was fooling myself if I thought I'd ever fully grasp this stuff. But now that I've had time to sit and read through it, it makes sense, which is encouraging. It's simply beyond the scope of where I'm at in the book right now so I'll have to look up some of the concepts and read more about it. That being said, starting next week I'm going to second shift and hopefully will have more time and a lot more mental energy to tackle this thing head on and continue toward my goal. I do have specific responses to people's replies, as well as more questions, but I have errands to run and don't have time right now to get into it. That being said, I at least wanted to give this quick reply and thank you all for your input. I'll be coming back to this thread as soon as I get a moment to sit down so we can discuss this further before I move on to the next chapter in the book.

[dutch]

Grime wrote:

can I ask if there is a specific reason why you're fitting the increment number (the first line of the file) to 5 characters?

The reason it fits into a fixed number of characters is so that you can overwrite it with larger numbers (up to that many digits).

Grime wrote:

One thought, suppose you're in the 1000th word. Then your program would have to extract 999 words before getting the 1000th word. Is there no better way?

Yes. I said at the end that you could store them in a binary file where every word takes up the same amount of space (say 20 characters). Then you could just seek to the exact spot without reading the previous words. That would be the proper way to do it. (Although reading through a thousand words isn't really a problem.)

Grime wrote:

Also, in the second approach there's one flaw that every time you run the program you get the same list of words. So you would have to add randomness.

No. It stores the current n in the file, so every time you run the program it starts where it left off the last time. It even wraps around when it hits the end of the words (without needing to store the number of words in the file).

[jonnin]

One thought, suppose you're in the 1000th word. Then your program would have to extract 999 words before getting the 1000th word. Is there no better way?

There are other ways, of course. You can treat it as a binary file, grab a random offset into it, find the delimiter (end of line char(s)?) and peel off a random word (this will take a little more code to handle the first and last word 'problem'). Its not better, just different. Until the file is QUITE large, you can just read it into memory as one big chunk and do the same thing (find a random offset and peel out a word). I would have done it the second way (read into one big chunk in memory).

[Grime]

Jonnin are you saying to parse every character and keep track of \n? Yes that wouldnt be much better than extracting every word. But what about setting all words to be fitted in 20 spaces (or whatever is the size of the largest word) like Dutch said and using that fact along with seekg() so that you can read only the word you desire?

file. seekg(some_value, 21*which_word)

cin >> word;

[MrCluckyToYou]

Grime wrote:

Some people will say that you should have even lesser code in main function but I can't critique on that.

I think you're right, but keep in mind I'm following through a book, and this chapter was about functions. He asked us to re-write his program by adding in two specific functions, since the previous version had everything in main().

FurryGuy wrote:

The biggest thing that stands out for me as being somewhat inefficient is shuffling the container of words.

Seems to be the general consensus here, and as dutch pointed out, random_shuffle no longer exists anyway. Perils of learning from outdated source material... Good thing we have these forums!

jib wrote:

It also doesn't make much sense to const qualify a parameter sent by value. And note that passing std::string by value can be an expensive operation and you should consider passing them by reference/const reference.

Does it not make sense because using references is the better way? Or because it's not good practice to send any constants by value? Again, I'm working from a book and that's how the author set it up, his argument being that once chosen, the secret word won't be changing. Therefore, should THE_WORD not be constant? Or should I have a different solution for checking the user input against the word (I'm guessing pass the reference to the function instead of the string value)?

I'm still confused, all these years later, by the difference between iterators, references, and pointers, and when to use which. From everything I've read, so is every other newbie. I've even gone to the point of investigating why C++ has pointers in the first place to see if it would help me understand. I would be eternally grateful to anyone who can point me towards a resource that can finally clear this up for me.

dhayden wrote:

Keep related code together.

Originally I had:

// main loop
	while (wrong < MAX_WRONG && soFar != THE_WORD)
	{
                used += guess;
		guess = getGuess(used);

But I just had this nagging feeling that updating the string of used characters should be done after getting the user's guess and checking it. I suppose it doesn't really matter, and you're right - keeping it together makes it clearer.

dhayden wrote:

Getting and validating input is a perfect example of a loop where the exit condition is really in the middle. Don't be afraid to code it that way.

I think I see what you mean. It's an infinite loop that only exits/returns if the user enters a character that hasn't been used. Is that for clarity's sake? Also, I hear what you're saying about the braces. Which brings up another question. You made some other remarks about commenting my code, and as far as formatting and commenting (i.e. "coding style") goes, I've read so much conflicting opinion on the matter. What ultimately sets the standard in a professional or team setting?

Thanks again everyone, especially you, dutch - for going above and beyond and challenging me to think!

[jlb]

Does it not make sense because using references is the better way? Or because it's not good practice to send any constants by value?

Well it really doesn't make sense because when you pass by value the compiler makes a copy of the variable so any change that is made in the function doesn't affect anything outside the function. For non-POD types it is also considered a good practice to pass by reference/const reference to avoid the unnecessary copy.

Edit:

I've even gone to the point of investigating why C++ has pointers in the first place to see if it would help me understand.

Well C++ has pointers for several reasons. The first reason is because C has pointers and C++ is a direct descendant of C. Another reason is that in C++ polymorphism usually requires pointers.

I'm still confused, all these years later, by the difference between iterators, references, and pointers, and when to use which.

These concepts are very similar and are basically a pointer with limitations. For example iterators are normally associated with containers and are used to iterate through a range, the limitations are usually iterator invalidation when the contents of the container change. A reference is an abstraction of a pointer that has limitations, for example a reference must be referencing a valid object. It's true asset, IMO, is that when used to with function parameters a reference eliminates the need to manually deference the variable. To truly understand any of these concepts understanding pointers, a somewhat complicated subject, is usually required.

[dhayden]

The primary use for C++ pointers is to let you do dynamic memory allocation. If you don't know how big a piece of data will be at compile time then you must allocate the space at runtime and pointers let you keep track of that space.

In C, pointers were also used to walk through dynamically allocated data structures like lists and trees.

Iterators let you walk through a container of some sort. They provide a consistent interface for doing so. To make the interface familiar, iterators use operator*() to access the item that an iterator currently points. to. Maybe this why you get confused between iterators and pointers: they both use * to access the underlying item. Even though iterators may be implemented via a pointer, that's hidden from the user. I think it's best to remember that operator*() is usually overridden by the iterator.

A reference is a synonym for another variable. If you think about it this way then a few things become clear. You can't modify a reference to refer to another variable, just as you can't change int a to somehow refer to a different integer. references are sometimes implemented by using a pointer, but that's entirely a compiler decision.

When should you use a pointer and when should you use a reference? I follow these rules:
- If "no object" is a legal possibility, then use a pointer (because pointers can be null).
- If you need to change it to refer to different objects, then use a pointer.
- If it should always refer to the same non-null object, then use a reference.

Dave

[jonnin]

Jonnin are you saying to parse every character and keep track of \n?

nah.

If you read the whole file into a blob of memory, say its 1 megabyte of words.
pick your favorite random location. say its location 1234 (in bytes). say that at that location we have "orite\random\n" -- the back end of the word favorite. Starting at your chosen location, just iterate... find \n, and capture the location after it, the r in random. find the next \n, and capture the location before it. Take that as a substring, and you get a string = "random".
play a round of the game using the word "random". Repeat, next time pick location 5678, ... etc. The only weirdness is making sure to handle what to do to allow access to the first and last words of the file, and not going off the memory if you picked the middle of the last word. That isnt too much extra logic, just gotta remember to do it (you can just wrap around to the beginning if you go off the back end).


So you are only parsing just a few chars to extract a word, at the cost of wasting a bit of memory to hold the file contents all the time. And whats a megabyte today anyway... 16gb is 16000 MB, its 1 of those...

The fixed width approach is nice and clean too. It wastes a tiny bit of disk space. I would say both solutions are equally good.

[Grime]

It wastes a tiny bit of disk space.

Hmm but you do need to store your words somewhere. Not like you can magically create a thousand words in RAM just like that without reading from somewhere. I guess you could say that you can scrape the words off the internet or something so it then it just becomes about preference.

[jonnin]

you seem to not be getting what I am saying.
if you have 1 million words, average length 5, but max length 20, and store them all in 20 bytes each, that takes up 20 million bytes.

if you store them one line each, whatever their length is, as 'text',
they take up about 5 million bytes. Right? This is the downside to fixed length access -- its more efficient but wastes a little space.

You have a file in any of the approaches listed. Its the size of the file that changed.

[Grime]

Right okay. But we don't need to fix all words to the max chars. We could store the frequency of the particular lengths of the word like I had originally suggested.

We might lose what 40 or a maximum of 60 bytes for writing this info at the beginning and maybe a few more math operations.

So this has about the same disk space and at the same time occupies no RAM.

[E0p9LyTq]

MrCluckyToYou wrote:

I'm working my way through a book called Beginning C++ Through Game Programming, Third Edition

I have the 2014 edition of the book and it is has been interesting learning to adapt the older C++11 (and earlier) code to C++17 standards using Visual Studio 2017.

That is a real learning experience, upgrading and updating old code to a newer standard. For instance:

Dumping the old C library random functions, and using the C++ <random> library. Replacing std::random_shuffle with std::shuffle.

[MrCluckyToYou]

FurryGuy wrote:

I have the 2014 edition of the book and it is has been interesting learning to adapt the older C++11 (and earlier) code to C++17 standards using Visual Studio 2017.

Does Visual Studio monitor your code to let you know when something isn't up to the current standard? I use Eclipse with the MinGW compiler.