Please critique my source code - Craps Game

[bos1234]

Question comes from Modern C programming by King ch 9 qn 7
http://i41.tinypic.com/rtn9f5.jpg
http://i44.tinypic.com/w22hqg.png

This is one of the larger programs I have written to date. I am trying to learn good habits early on, hence, I would like some honest feedback/criticism on the code written. If the code is complete garbage say so, and suggest improvements. Its the only way to learn. Is there too much white space? Could my comments/variables be improved? Is there bugs in the program? Can it be broken etc etc?

Source code

/*
 * Craps game
 */

#include <stdio.h>
#include <stdlib.h>	//for srand and rand
#include <time.h>

int roll_dice(void);
int play_game(void);	//returns true or false

int main()
{
	int WINS = 0, LOSSES = 0, result;
	char play_again;

	srand((unsigned) time(NULL));

	do
	{
		result = play_game();	

		if (result == 0)
		{
			printf("You WIN!\n\n");
			WINS++;							//keeping track of wins
		}
		else if (result == 1)
		{
			printf("You LOSE!\n\n");
			LOSSES++;						//keeping track of losses
		}

		printf("Play again? ");
		play_again = getchar();

		getchar();						 //soak up \n

	} while (play_again == 'y' || play_again == 'Y');

	printf("\nWins: %d	Losses: %d", WINS, LOSSES);

	getchar();getchar();getchar();
	return 0;
}

int roll_dice(void)
{
	int first_roll, second_roll, sum;

	first_roll = rand() % 7;
	while (first_roll == 0)				//if 0 shows up, roll again
		first_roll = rand() % 7;	

	second_roll = rand() % 7;
	while (second_roll == 0)			//if 0 shows up, roll again
		second_roll = rand() % 7;

	sum = first_roll + second_roll;

	return sum;
}

int play_game(void)		//return bool
{
	int sum, number_to_win;

	sum = roll_dice();

	printf("\nYou rolled: %d\n", sum);

	if (sum == 7 || sum == 11)
		return 0;						//0 is win

	else if (sum == 2 || sum == 3 || sum == 11)
		return 1;
	else
	{
		number_to_win = sum;
		printf("Your point is %d\n", number_to_win);
		
		do 
		{
			 sum = roll_dice();
			 printf("You rolled: %d\n", sum);

		} while (sum !=7 && sum != number_to_win);

		if (sum == 7)
			return 1;
		
		else if (sum == number_to_win)
			return 0;
	}
}

[anduril462]

int WINS = 0, LOSSES = 0, result;

Usually ALL_CAPS is reserved for constants -- it's just a convention, but a very widespread one. Make these lower case.

if (result == 0)
...
if (sum == 7 || sum == 11)
    return 0;                       //0 is win
else if (sum == 2 || sum == 3 || sum == 11)
    return 1;

Define constants called WIN and LOSE. Use them in place of magic numbers. Their value doesn't really matter, but 0 and 1 are fine.

printf("Play again? ");
play_again = getchar();
 
getchar();                       //soak up \n

Not very robust. What if I accidentally type "yy\n". The first getchar reads a 'y', the second soaks up the other 'y', and there's a '\n' stuck in the buffer for next time. I prefer to read a line with fgets, strip the new line, then check for 'y' or 'n' (or their upper case counterparts). If you want to be really strict, you also check that they entered exactly 1 character. I would even say you should define a function called play again. Something like:

bool play_again()
{
    do
        print "play again (y|n) ?"  // tells the user valid choices are y or n
        fgets
        clear the buffer, which should be a function on it's own
        strip newline
    } while not 'y', 'n', 'Y' or 'N'
    return true if user entered 'y' or 'Y'; else return false
}
...
do {
    // play game
} while (play_again());

This is not cool:

getchar();getchar();getchar();

I have to press 3 keys to exit your program, but you don't tell me. IMO, basic console programs should not do this even once. Just let the program end and go to the next command prompt.

int play_game(void)     //return bool

If you wanted to return a boolean type, make it return bool. But you don't. Just because you only have two possible values doesn't mean it's automatically a boolean. Think about how you use this return value. Do you ever intend for it to be part of some logical expression like play_game() && (x == 13)? No. You simply want to convey win/loss. Thus, returning int is fine, but use the constants I mentioned.

first_roll = rand() % 7;
while (first_roll == 0)             //if 0 shows up, roll again
    first_roll = rand() % 7;

That's silly. Just get a random number in the range of [0,5] and add 1 to it: (rand() % 6) + 1.

That was a pretty superficial review, I can't compile/run it right now, so I might have missed something.

[oogabooga]

Even though anduril has hit most of these points, I'll post my entire spiel.

It looks very good. Nicely formatted.

WINS and LOSSES should probably be lowercase since all uppercase is conventionally used to indicate a macro created with define.

Why is 0 a win and 1 a loss? It seems more natural to do it the other way around. If you're thinking of standard library functions that return 0 to mean "success", they do that so they have multiple values for errors. I.e., 0 is success, 1 is out-of-memory, 2 is can't open file, etc. I'd recommend using stdbool.h, which declares bool, true, and false, and return true for a win.

The getchar() at the end of the while loop to "soak up \n" should be a function (let's call it eatnewline) that calls getchar() until '\n' or EOF. Otherwise an extraneous space will choke your program. (Try entering "y ". You may need to do it a couple of times for it to choke.) But it's best to consume the whole line with fgets().

I'd write a play_again() function that prints the prompt, accepts the response, and ensures the input stream is newline-free. Have it return true or false and use it directly in the while condition.

With the above your mainline would be something like this:

do
{
    if (play_game())
    {
        // win
    }
    else
    {
        // loss
    }
} while (play_again());

play_game is missing a return at the very end. Compiling with a high warning level should point this out. (On gcc use -Wall.) I realize that this return will never be executed, but it's a good idea to write your code so there's no warnings.

I don't understand roll_dice. If you're trying to get a number between 1 and 6 it's usually done like this:

rand() % 6 + 1

The function can be simplified to:

int roll_dice(void)
{
    return rand() % 6 + rand() % 6 + 2;
}

In play_game, number_to_win is usually called "point" in craps.

You should add a newline to the end of your final printf. It's not ensured that the output will appear unless a newline is output.

If you really need to hold the console at the end, use the eatnewline function.

[anduril462]

You should add a newline to the end of your final printf. It's not ensured that the output will appear unless a newline is output.

Technically true, but to clarify:

5 The file may be subsequently reopened, by the same or another program execution, and
its contents reclaimed or modified (if it can be repositioned at its start). If the main
function returns to its original caller, or if the exit function is called, all open files are
closed (hence all output streams are flushed) before program termination. Other paths to
program termination, such as calling the abort function, need not close all files
properly.

The output stream is guaranteed to be flushed so long as his code takes a "normal" path to termination (i.e. main terminates or he calls exit). In his case, unless he seg faults somehow (which is very difficult given his code) or adds a call to abort, or kills it mid-way through a run with ctrl-c or something, the program will terminate and flush the streams. Regardless, a new line at the end is always good, since it ensures the prompt will be on it's own line afterward, especially if he removes all those getchar calls (thus nobody is pressing enter 3 times).

[oogabooga]

Technically true, but to clarify...

I knew that, but the program (with the console-holding stuff at the end) does not terminate right away.

Where did you get that C11 quote?

[anduril462]

> I knew that, but the program (with the console-holding stuff at the end) does not terminate right away.
Actually, I just remembered that requesting input on stdin (e.g. with a function like getchar, scanf, fgets) will cause stdout to be flushed. At least, I think that's the correct interpretation of the part I emboldened.

When a stream is unbuffered, characters are intended to appear from the source or at the
destination as soon as possible. Otherwise characters may be accumulated and
transmitted to or from the host environment as a block. When a stream is fully buffered,
characters are intended to be transmitted to or from the host environment as a block when
a buffer is filled. When a stream is line buffered, characters are intended to be
transmitted to or from the host environment as a block when a new-line character is
encountered. Furthermore, characters are intended to be transmitted as a block to the host
environment when a buffer is filled, when input is requested on an unbuffered stream, or
when input is requested on a line buffered stream that requires the transmission of
characters from the host environment. Support for these characteristics is
implementation-defined, and may be affected via the setbuf and setvbuf functions.

> Where did you get that C11 quote?
From the C11 standard . We have a draft copy here: C Draft Standards.

[oogabooga]

I just remembered that requesting input on stdin (e.g. with a function like getchar, scanf, fgets) will cause stdout to be flushed. At least, I think that's the correct interpretation of the part I emboldened.

Good point.


I'm not sure if that's what the quote actually says, but I have heard that before and it makes sense, at least when stdin and stdout are attached to a console.


But since stdin and stdout can be separately redirected I guess they must actually be different streams, so I don't think that receiving input on stdin would always flush stdout.

[gemera]

I researched this topic a few months back. Originally I was using the book C Plus by Stephen Prata who interpreted the standard in a way that agreed with andurils interpretation, which I was quite happy to follow. But then I discovered a whole other group of experts who were interpreting the same quoted standard to argue that it is 'implementation defined' and thus fflush(stdout) should be used.(Steven Summit is in this group which appears judging by the posts on the web to have become the accepted interpretation.).

There is also a subtle difference in the presentation of the standard (in c89/90 and c99 at least) where in some online copies the paragraph quoted by anduril has sections highlighted in grey and not in others.

Given that there appears to be no completely settled agreement I've decided to take the safe option and use fflush(stdout).

[anduril462]

Many years ago, when I first learned C in school (sometime between C89 and C99), I was taught that requesting input on stdin flushed stdout, no mention of implementation-defined behavior. Every implementation I had used (for hosted environments at least) followed that behavior, so I guess I took it for granted (I didn't even realize there was a standard back then).

Really ISO/IEC JTC1/SC22/WG14? Could you have made this any more complicated?

There's 7.19.3p3, which I mentioned above, which says it's implementation-defined. But then, there's this (which is in the "hosted environment" section):

The least requirements on a conforming implementation are:
— Accesses to volatile objects are evaluated strictly according to the rules of the abstract
machine.
— At program termination, all data written into files shall be identical to the result that
execution of the program according to the abstract semantics would have produced.
— The input and output dynamics of interactive devices shall take place as specified in
7.19.3. The intent of these requirements is that unbuffered or line-buffered output
appear as soon as possible, to ensure that prompting messages actually appear prior to
a program waiting for input.

which basically says "well, 7.19.3 says behavior is implementation-defined, but really, that's only for non interactive devices. We have fully defined the behavior on interactive devices, so requesting input will definitely flush output. That way, you get the prompt before you enter input, which is what every programmer and user in the world wants". But that is immediately followed by

What constitutes an interactive device is implementation-defined.

So that basically says "That implementation-defined behavior? You know, the one we fully defined in specific (interactive devices) cases so you can rely on it? Yep, we are leaving 'interactive devices' as implementation-defined, so go f*** yourself, you can't rely on anything". Why not just say "Whether output is flushed before input is requested is implementation-defined." That's it. One sentence is all they needed, in one location in the standard, to clear up all doubt on this issue.

But now I think you're right gemera, you aren't guaranteed that the implementation will do it for you, so explicitly calling fflush(stdout) is the safest and most portable behavior. That is really obnoxious however, to have to flush stdout before every input, which is probably why most (all?) implementations do it for you.

EDIT: I hope nobody on C Board is on the C standards committee, so...present company excluded? Honestly though, they did good, on a tough job. I don't envy them and I'm sure I couldn't do any better myself.

EDIT 2:
Just found this:

The class of interactive devices is intended to include at least asynchronous terminals, or paired
display screens and keyboards. An implementation may extend the definition to include other
input and output devices, or even network inter-program connections, provided they obey the
Standard’s characterization of interactivity.

So it appears they had specific definitions for interactive devices in mind, but they don't actually specify them in the standard. Instead, you have to read the "why we did what we did" document, which is not actually authoritative, so this still doesn't count, it's still implementation defined.