Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

This is a really good effort for a first major project! It's very straightforward and easy to understand. Here are a few ways you could take it to the next level.

Avoid Global Variables

#Avoid Global Variables RightRight now you have 4 global variables:

int balance;
int pot;
int deck[52];
int sdeck[52];

These should be local variables inside the main() function and you should pass them to the other functions that need to access them. There are 2 reasons:

  1. As it is now, it's difficult to find who changed them when they change
  2. If you ever want to expand this (perhaps to be a server that serves games to multiple groups of people at the same time), having globals won't work.

Avoid Magic Numbers

#Avoid Magic Numbers YouYou created a constant CARDS but then you only use it once. You should use it to declare your decks, too (and I'd rename it to NUM_CARDS):

int deck[NUM_CARDS];
int sdeck[NUM_CARDS];

While you're unlikely to ever need to change the number of cards in a deck, using a constant can clarify the code and save you from typos. (And actually, there are a few games that use fewer than 52 cards).

Use Arrays for Looking Things Up

#Use Arrays for Looking Things Up InIn your csuit() and cface() functions, you have a bunch of case statements to convert between an int and a string. You could do this more easily with an array, like this:

const char* csuit(const int v)
{
 const char* kSuits[] = {
 "Hearts",
 "Clubs",
 "Diamonds",
 "Spades"
 };
 return kSuits [ v % 4 ];
}

You can do a similar thing with cface().

Simplify

#Simplify SeveralSeveral of your functions have complicated nested while loops. I'd try to simplify them. I'd make the one in main() it more like this:

while (strcmp(input, "quit") != 0)
{
 if (strcmp(input, "play") == 0)
 {
 play();
 }
 else if (strcmp(input, "help") == 0)
 {
 printf("Type 'play' to begin the game.\n");
 }
 scanf("%s", input);
}

You can eliminate the call to quit() as your main() will just exit when the user enters "quit".

On the subject of simplifying, I'd probably also break turn() into smaller functions, and try to make the logic simpler. Perhaps use a state machine.

This is a really good effort for a first major project! It's very straightforward and easy to understand. Here are a few ways you could take it to the next level.

#Avoid Global Variables Right now you have 4 global variables:

int balance;
int pot;
int deck[52];
int sdeck[52];

These should be local variables inside the main() function and you should pass them to the other functions that need to access them. There are 2 reasons:

  1. As it is now, it's difficult to find who changed them when they change
  2. If you ever want to expand this (perhaps to be a server that serves games to multiple groups of people at the same time), having globals won't work.

#Avoid Magic Numbers You created a constant CARDS but then you only use it once. You should use it to declare your decks, too (and I'd rename it to NUM_CARDS):

int deck[NUM_CARDS];
int sdeck[NUM_CARDS];

While you're unlikely to ever need to change the number of cards in a deck, using a constant can clarify the code and save you from typos. (And actually, there are a few games that use fewer than 52 cards).

#Use Arrays for Looking Things Up In your csuit() and cface() functions, you have a bunch of case statements to convert between an int and a string. You could do this more easily with an array, like this:

const char* csuit(const int v)
{
 const char* kSuits[] = {
 "Hearts",
 "Clubs",
 "Diamonds",
 "Spades"
 };
 return kSuits [ v % 4 ];
}

You can do a similar thing with cface().

#Simplify Several of your functions have complicated nested while loops. I'd try to simplify them. I'd make the one in main() it more like this:

while (strcmp(input, "quit") != 0)
{
 if (strcmp(input, "play") == 0)
 {
 play();
 }
 else if (strcmp(input, "help") == 0)
 {
 printf("Type 'play' to begin the game.\n");
 }
 scanf("%s", input);
}

You can eliminate the call to quit() as your main() will just exit when the user enters "quit".

On the subject of simplifying, I'd probably also break turn() into smaller functions, and try to make the logic simpler. Perhaps use a state machine.

This is a really good effort for a first major project! It's very straightforward and easy to understand. Here are a few ways you could take it to the next level.

Avoid Global Variables

Right now you have 4 global variables:

int balance;
int pot;
int deck[52];
int sdeck[52];

These should be local variables inside the main() function and you should pass them to the other functions that need to access them. There are 2 reasons:

  1. As it is now, it's difficult to find who changed them when they change
  2. If you ever want to expand this (perhaps to be a server that serves games to multiple groups of people at the same time), having globals won't work.

Avoid Magic Numbers

You created a constant CARDS but then you only use it once. You should use it to declare your decks, too (and I'd rename it to NUM_CARDS):

int deck[NUM_CARDS];
int sdeck[NUM_CARDS];

While you're unlikely to ever need to change the number of cards in a deck, using a constant can clarify the code and save you from typos. (And actually, there are a few games that use fewer than 52 cards).

Use Arrays for Looking Things Up

In your csuit() and cface() functions, you have a bunch of case statements to convert between an int and a string. You could do this more easily with an array, like this:

const char* csuit(const int v)
{
 const char* kSuits[] = {
 "Hearts",
 "Clubs",
 "Diamonds",
 "Spades"
 };
 return kSuits [ v % 4 ];
}

You can do a similar thing with cface().

Simplify

Several of your functions have complicated nested while loops. I'd try to simplify them. I'd make the one in main() it more like this:

while (strcmp(input, "quit") != 0)
{
 if (strcmp(input, "play") == 0)
 {
 play();
 }
 else if (strcmp(input, "help") == 0)
 {
 printf("Type 'play' to begin the game.\n");
 }
 scanf("%s", input);
}

You can eliminate the call to quit() as your main() will just exit when the user enters "quit".

On the subject of simplifying, I'd probably also break turn() into smaller functions, and try to make the logic simpler. Perhaps use a state machine.

Source Link
user1118321
  • 11.9k
  • 1
  • 20
  • 46

This is a really good effort for a first major project! It's very straightforward and easy to understand. Here are a few ways you could take it to the next level.

#Avoid Global Variables Right now you have 4 global variables:

int balance;
int pot;
int deck[52];
int sdeck[52];

These should be local variables inside the main() function and you should pass them to the other functions that need to access them. There are 2 reasons:

  1. As it is now, it's difficult to find who changed them when they change
  2. If you ever want to expand this (perhaps to be a server that serves games to multiple groups of people at the same time), having globals won't work.

#Avoid Magic Numbers You created a constant CARDS but then you only use it once. You should use it to declare your decks, too (and I'd rename it to NUM_CARDS):

int deck[NUM_CARDS];
int sdeck[NUM_CARDS];

While you're unlikely to ever need to change the number of cards in a deck, using a constant can clarify the code and save you from typos. (And actually, there are a few games that use fewer than 52 cards).

#Use Arrays for Looking Things Up In your csuit() and cface() functions, you have a bunch of case statements to convert between an int and a string. You could do this more easily with an array, like this:

const char* csuit(const int v)
{
 const char* kSuits[] = {
 "Hearts",
 "Clubs",
 "Diamonds",
 "Spades"
 };
 return kSuits [ v % 4 ];
}

You can do a similar thing with cface().

#Simplify Several of your functions have complicated nested while loops. I'd try to simplify them. I'd make the one in main() it more like this:

while (strcmp(input, "quit") != 0)
{
 if (strcmp(input, "play") == 0)
 {
 play();
 }
 else if (strcmp(input, "help") == 0)
 {
 printf("Type 'play' to begin the game.\n");
 }
 scanf("%s", input);
}

You can eliminate the call to quit() as your main() will just exit when the user enters "quit".

On the subject of simplifying, I'd probably also break turn() into smaller functions, and try to make the logic simpler. Perhaps use a state machine.

lang-c

AltStyle によって変換されたページ (->オリジナル) /