Archived

This topic is now archived and is closed to further replies.

popcorn

feedback on my code

Recommended Posts

Hi, I've just written some code for a simple game of hangman and would appreciate some feedback on the code and improvements etc. Written using C on the Dev C++ 4.9.8.4 IDE
#include <stdio.h>
#include <stdlib.h>

//constants

#define MAXSTRING 20
#define LIVES 3

//function prototypes

void flush();
char *generateword();

typedef char WORD[MAXSTRING];
WORD words[5] = {"helloworld", "diablo", "gta", "warcraft", "starcraft"};

int main(int argc, char *argv[])
{
  char word[MAXSTRING] = {0};//holds the word that player must match

  char word2[MAXSTRING] = {0};//the word that the player sees

  
  char guess;//holds the letter that the player guessed

  char play;//holds player response for playing again(y or n) 

  
  int status;//flag for determining whether a letter has been found in the word(0 = found or 1 = not found) 

  int lives = LIVES;//holds the number of lives that the player has left

  int i;//looping variable 

  
  strcpy(word, generateword());
  strcpy(word2, word);
  
  //create the blanks in the word

  for(i=0; i<strlen(word); i++)
  {
    word2[i] = '*';
  }
  
  printf("Hangman\n");
  printf("=======\n\n");
  
  //start gameloop

  do
  {   
    printf("%s\n", word2);
  
    printf("Enter a letter: ");
    scanf("%c", &guess);
    flush();
    
    for(i=0; i<strlen(word); i++)
    {
      if(guess == word[i])
      {
        status = 0;
        break;
      }
      else
        status = 1;
    }
    
    switch(status)
    {
      case 0: 
      for(i=0; i<strlen(word); i++)
      {
        if(guess == word[i])
          word2[i] = guess;
      }
      break;
      case 1:
        lives--;
      break;
    }
    
    //check to see if the player has guessed all the letters

    if(strcmp(word, word2) == 0)
    {
      printf("Congratulations, you have won\n");
      printf("word is %s\n", word);
      printf("word2 is %s\n", word2);
      printf("Play again(y/n): ");
      scanf("%c", &play);
      flush();
      if(play == 'y')
      {
        lives = LIVES;//set lives back to normal

        
        strcpy(word, generateword());
        strcpy(word2, word);
  
        for(i=0; i<strlen(word); i++)
        {
          word2[i] = '*';
        }
      }
    }
    
    //check to see if the player has run out of lives

    if(lives == 0)
    {
      printf("Sorry you lost\n");
      printf("word was %s\n", word);
      printf("word2 is %s\n", word2);
      printf("Play again(y/n): ");
      scanf("%c", &play);
      flush();
      if(play == 'y')
      {
        lives = LIVES;//set lives back to normal

        
        strcpy(word, generateword());
        strcpy(word2, word);
  
        for(i=0; i<strlen(word); i++)
        {
          word2[i] = '*';
        }
      }
    }
    
    printf("%s\n", word2);  
    printf("Lives left %d\n", lives);           
  }while(play != 'n');
   
  system("PAUSE");	
  return 0;
}

//function for carraige return problem

void flush()
{
  char ch;
  while((ch = getchar()) != '\n');
}

//function to randomly generate a word

char *generateword()
{
  int numofwordsinarray, num;
  
  numofwordsinarray = sizeof(words) / sizeof(WORD);
  srand((unsigned)time(NULL));
  num = rand() % numofwordsinarray;
  
  return words[num];
}

   
How about them apples? [edited by - popcorn on January 5, 2004 4:03:53 PM] [edited by - popcorn on January 5, 2004 4:04:33 PM] [edited by - popcorn on January 5, 2004 4:05:39 PM]

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
I don''t know your level of experience, so I will just throw out some general advice. First, your main() function is doing a lot of work. The code would be much easier if you used a bit more abstraction (create sub routines to do different tasks). Also, you could relace some of those for loops where you are resetting strings your a simple strcpy call. I''m sure there are other nit picky things that others will cover . Looks good, you should feel happy to finish a project. That''s the really important part.

Share this post


Link to post
Share on other sites
Congratulations popcorn! Good job. Nicely done.

As the AP said, splitting your program up in to sub routines will give you some advantages. Mainly the code will be easier to read, secondly it will be much much easier to enhance.

My major critque is that you''ve tied the user initialization, user interface, and game logic together. What would you do if you wanted to create a graphical verison? My guess is that you would need to re-write most of your game logic. If you put the game logic in to a separate functon call (like you did with flush()) it would be easier to add extra layers for a different interface.

I like your use of constants and the typedef. Very professional. It''s obvious that you understand the idea behind these things. As you gain more experience you''ll probably use different naming conventions, but that''s something that comes with time.

I like that you made generateword a separate function. generateword returns a pointer to an index in your dictionary, which I think is a smart way of approaching the problem.

Something you could try adding is a file-based word dictionary. This way your program could randomly pick a word out of a file. If you do this, try finding a way that will require very little modification to your main loop.

Cheers,
Will

Share this post


Link to post
Share on other sites
Here''s a few more comments. If you''re program doesn''t make use of any arguments passed to it you can just int main(). You might also want to move the srand(time(NULL)) outside of your generateword function -- like for example, at the very start of the program. You don''t want srand() to keep reseeding everytime you call generateword.





--{You fight like a dairy farmer!}

Share this post


Link to post
Share on other sites
Thanks for the feedback, its always good to see what other people think about your code and where you can improve.

I will try to make a version which uses a textfile to store all the words next RPGeezus and then try and break the game down into more parts like you and AP described.

I totally agree about the srand function, Greatwolf. It shouldn''t be part of the generateword function.

As for my level of experience it would be hard to say. Although I''ve posted this on the beginners forum I would say that I''m not a total beginner. I have spent quite a bit of time reading books and articles on the net and have been dipping in and out of programming for a while(mainly c and c++). I have also had a look at windows and direct x programming, however I think that I was jumping the gun by trying to approach those topics(I barely understood how to program then).

Like you said AP its been nice to actually complete a project and to have something to refer back to in future projects.

How about them apples?

Share this post


Link to post
Share on other sites
Nice!

Just a nitpick, maybe your generateWord() function could use a name change, you don''t actually "generate" a word, you select one at random.

Keep on the good work.

Share this post


Link to post
Share on other sites
Hey, great job on a finished project!

MY advice is to get rid of this:

typedef char WORD[MAXSTRING];

for a couple reasons. First, typedefing something to an array is something I have tried in the past and found to be not worth it. For one thing, you can''t use the new type to overload a function. As in, this code:

typedef float float3[3];
typedef float float4[4];

void normalize(float3 f) {
//blahblah
}
void normalize(float4 f) {
//blahblah
}

will not work, even though it feels like it should. (the reason is that all arrays devolve into simple pointers when used as arguments)

Reason 2: you don''t need to set the maximum size for those strings, because you are giving it string literals, and the compiler already gives it enough size. You can change this line:

WORD words[5] = {"helloworld", "diablo", "gta", "warcraft", "starcraft"};

to

char *words[5] = {"helloworld", "diablo", "gta", "warcraft", "starcraft"};

or even

char *words[] = {"helloworld", "diablo", "gta", "warcraft", "starcraft"};

and reason 3: I''m pretty sure the type "WORD" is defined already, probably in windows.h (because it has a special meaning related to data size)

Share this post


Link to post
Share on other sites
Those are some good points pinacolada - its something I will bear in mind in the future. I guess it would also be more efficient to use your method since the array would take up less bytes(20 as opposed to the 100 currently used for the array).

I know word is already defined but I couldn't really be bothered to think up of another word at the time(dictionary would probably be more appropriate) but thanks for the advice.

How about them apples?

[edited by - popcorn on January 6, 2004 9:21:56 PM]

Share this post


Link to post
Share on other sites