C code critique?

[Jerry]

As you will see (if it's not the case already), I'm extremely bored at the moment.

So I'll show a little console game I wrote as part of an online course on C programming. One exercise in said course was to write a console game where a random number is generated and you have to guess it. The program then gave you the number of tries you took to guess. The first time I achieved to get a semi-working code with nothing more than what was asked. Invalid inputs were mostly not handled, so it was super easy to crash the program.

Then a few months ago by curiosity I revised my C a bit and looked back at my code. Within an evening of (non-continous) work I rewrote it from scratch, took care of most of the bugs, and added difficulty levels. Took me a few tries but each time finding the culprit was a manner of minutes.

For your viewing pleasure, here's the code. If you're curious to see the actual thing, you can paste it in an empty Visual C++ project. It should compile and run.

// More or less - v0.4
//
// Less-than-clean code ahead. Beware.
//
// Quickly translated for clarity. Original code had french
// commentary. Function and variable names are still a mix of
// french and english.


#include <StdAfx.h>
#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <time.h>

int NewGame(int *PointerMAX, int *PointerMIN, int *PointerNmbreRand, int *PointerNmbreEntree);

int main(int argc, char *argv[])

{
    int MIN = 1; //Minimum for generated number, constant
	int MAX = 100;
	int NmbreRand = 0;
	int NmbreEntree = 0;
	int NmbreCoups = 0;
	int ContinueGame = 1;

	srand(time(NULL)); //Initializing random number generator

	printf("Welcome to More or Less!\n\n");

	while(ContinueGame != 0)
	{
		NmbreCoups = NewGame(&MAX, &MIN, &NmbreRand, &NmbreEntree);

	    printf("Number has been found in %d tries!\n\n", NmbreCoups);
		printf("Continue? 0 to leave, another number to continue.");
		scanf("%d", &ContinueGame);
		while(getchar() != '\n');

	}

	system("PAUSE");

	return 0;
}

int NewGame(int *PointerMAX, int *PointerMIN, int *PointerNmbreRand, int *PointerNmbreEntree) //Function of the game itself
{
	double Choix = 0;
	double PowResult = 0;
	int Counter = 0;

	printf("Select a difficulty level:\n\n");
	printf("1) EASY - BETWEEN 1 AND 100\n\n");
	printf("2) MEDIUM - BETWEEN 1 AND 1000\n\n");
	printf("3) HARD - BETWEEN 1 AND 10 000\n\n");
	printf("Your choice:");
	scanf("%lf", &Choix);
	while(getchar() != '\n');

	if(Choix < 1 || Choix > 3) //Checking the input
	{
		printf("Invalid choice. Default level set (EASY)\n\n");
	}
	else
	{
	    PowResult = pow(10, Choix + 1); //If valid, setting the MAX limit
		*PointerMAX = PowResult;
	}

    *PointerNmbreRand = (rand() % (*PointerMAX - *PointerMIN + 1)) + *PointerMIN; //Generating the number

	printf("A number between 1 and %d has been generated! What is it?", *PointerMAX);

	while(*PointerNmbreEntree != *PointerNmbreRand)
	{
		scanf("%d", PointerNmbreEntree);
		while(getchar() != '\n');

        if(*PointerNmbreEntree < 1 || *PointerNmbreEntree > *PointerMAX) //If input less than 1 or greater than MAX
		{
			printf("\nYou must give a number between 1 and %d! Try again.", *PointerMAX);
		}
		
		else if(*PointerNmbreEntree < *PointerNmbreRand)
		{
			printf("\nThe number is greater!");
			Counter ++;
		}
		else if (*PointerNmbreEntree > *PointerNmbreRand)
		{
			printf("\nThe number is smaller!");
			Counter ++;
		}

	}
		printf("\nExact! It was the number %d!\n", *PointerNmbreRand);

	return Counter;
}

I think there's still a little bug in this program. But I don't remember clearly.

My coding experience is very limited, so I'm pretty sure there are cleaner and better ways to code this little game. But again, this is just one evening of work. I could've put the difficulty level selection in a loop instead of setting the default level and move on, but meh, didn't bother with that LOL.

So I'm curious to know. How's that for a first complete, pretty-much-working program? Don't be afraid to point out flaws, this is meant to be constructive!

[Sasha'rgo]

 

-pausing to read code.

"Counter" should increment on any attempt, because the winning attempt is still an attempt. A first attempt win for example would report 0 attempts at the end of the game. (Counter = 0, guess is right, game loop skipped, game function returns 0.)

- do you require the game to exist within a function? You might only need to return the score now, however, what if you want greater control? You mix sending pointers as references and using a return even though they perform the same duties, passing a result back a scope in to main()'s variables. Maybe reading further on variable scopes and how functions might best apply to certain cases where a more local scope is required might be useful to you, to understand what is necessary to write.

- Within the game, no option is presented to stop guessing and return to menu, the loop is based on the condition of winning when it might be best served as an inner loop, and the game loop exiting on a non-integer or specific keyword to quit the current round for example if it is too difficult etc.

- An array of integers can store the difficulties, such as int num[] = {100, 1000, 10000} and referenced by zero-index, i.e. if the user enters 1, subtract 1 and it becomes 0, and the first index for num, num[0], is '100' as intended. Data structures such as structs can store more data along with the index such a strings, titles or map data, if required.  I know you mentioned you could have done this, however.

- Within the game, you use scanf however ignore its results. You ask it to consume a decimal (integer), however if something else is entered, the game could enter some condition that ruins the game state. while(scanf("%d", ...) == 0) { ...printf("Please enter a decimal..");.. } (i.e. if 0 %d was found, warn the user and try again before continuing might be more robust.)

- A compiler with a debugger is your friend, it can walk you through each line of code, you can add a breakpoint before and during a loop to see where something goes wrong and your console quits, and even 'watch' variables and their states and even their history while each input is being processed Essential for any programming, absolutely.

- your custom random function might be best put in a function itself, however note that rand() itself will not serve you numbers over RAND_MAX  (i.e. 32767 on my Windows 7 machine with a MinGW/GCC compiler.) This is only ~3x larger than your maximum difficulty of 10,000, and so a 100,000 option will certainly never return beyond 33k if it existed. Your random function might best return a float within a normalised range between 0 and 1 exclusive, and multiply that by your desired range, i.e. rand() / RAND_MAX  makes 0.0-1.0, and then times 100,000 scales RAND_MAX to 100,000. How any of these maths perform, is your tradeoff to consider.

 Ncurses might be something enjoyable to play with, if the game was an eventually continued project:

http://www.tldp.org/HOWTO/NCURSES-Programming-HOWTO/menus.html

Just some nit-picking, C is not pretty from scratch so I could not suggest more than abstract ideas of how I might do it.. I am certain with some menu/data structures, more robust logic and error handling, you might come up with something where more diverse input libraries, objects, classes and templating ala C++ are not necessary. It might be easier to just go for that, however, I can tell you've learned the basics of C and its hackery which is useful.

~A tiger's thoughts at 1:30am..Welp.

[Sergalus]

Ahhh I wrote this game in python year ago, nice to see it on c. Well I already forgot most things about c damn pointers.

[fennecbyte]

4 hours ago, Sergalus said:

damn pointers.

Pointers are great, tho.