Skip to main content
Spelling, grammar, markdown
Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

The Good
##The Good The choice of variable and function names is very descriptive. The code is properly indented but. The consistent use of camelCodecamelCase is excellent.

MAGIC NUMBERS
##MAGIC NUMBERS The term Magic Numbers refers to numerical constants in the code. A good programingprogramming practice is to used named constants rather than numbers. Named constants make the code more self documenting, and allow easier modification of the code. When a named constant is used, the code only needs to be changed in one location rather than multiple locations. An example of this in the code would be to increase or decrease the size of the arrays.

Note: The code above would be more efficient if you had an array of color strings and indexed into the the array using randColorrandColor as follows:

Not only does it make the function shorter, but indexing into the array is faster than the switchswitch statement.

@MegaTom is correct,correct; this code would be better using enums or named constants. The string compares are much less efficient that integer compares.

Multiple Statements on a Line ##Multiple Statements on a Line
To make future modifications easier, there should never be multiple statements on a single line. Let's say some code needed to be added to each case in the switchswitch statement. Each of the cases in the switchswitch statement would then be need to be broken into multiple lines which makes the edit more complex, and can create typos during the edit. It is much easier to just add another statement by adding a single line, rather than trying to add it to a single line.

Prefer return() over exit()
In the case of this program since there is no error checking there is no reason to use either ##Prefer return over exit()
In the case of this program, since there is no error checking, there is no reason to use either exit() or return(). If there is error handling in the program, the exit() function should only be used if the program encounters an error it can't correct deep in multiple function calls. The exit() function should only be used in stand alonestandalone programs, never in operating systems code. When using the exit()exit() function, use the macros EXIT_SUCCESSEXIT_SUCCESS and EXIT_FAILUREEXIT_FAILURE that are defined in stdlib.h<stdlib.h>.

Don't Repeat Yourself ##Don't Repeat Yourself
When code is repeating itself, it is better to write another function rather than repeating the code. Then the code only needs to be written and debugged once rather than multiple times. This is knowknown as the DRY principlein in software engineering.

Example###Example: The following code has loops that repeats that repeat:

Decrease Function Complexity ##Decrease Function Complexity
Another software engineering principle is the Single Responsibility Principle. A function should only be responsible for one action,action; this makes each function easier to read, write, debug and use multiple times. It is much better to write smaller more concise functions() so that the can be used in multiple places and only need to be debugged once. The functions codeCheck() and main() would both benefit from applying this principle.

General Comments ##General Comments
The program should guide the user better. It's not clear what the input should be,be; there should be prompts for each input. The program might be more fun if the user could enter colors rather than numbers.

The Good
The choice of variable and function names is very descriptive. The code is properly indented but. The consistent use of camelCode is excellent.

MAGIC NUMBERS
The term Magic Numbers refers to numerical constants in the code. A good programing practice is to used named constants rather than numbers. Named constants make the code more self documenting, and allow easier modification of the code. When a named constant is used the code only needs to be changed in one location rather than multiple locations. An example of this in the code would be to increase or decrease the size of the arrays.

Note: The code above would be more efficient if you had an array of color strings and indexed into the the array using randColor as follows:

Not only does it make the function shorter, but indexing into the array is faster than the switch statement.

@MegaTom is correct, this code would be better using enums or named constants. The string compares are much less efficient that integer compares.

Multiple Statements on a Line
To make future modifications easier, there should never be multiple statements on a single line. Let's say some code needed to be added to each case in the switch statement. Each of the cases in the switch statement would then be need to be broken into multiple lines which makes the edit more complex, and can create typos during the edit. It is much easier to just add another statement by adding a single line, rather than trying to add it to a single line.

Prefer return() over exit()
In the case of this program since there is no error checking there is no reason to use either exit() or return(). If there is error handling in the program, the exit() function should only be used if the program encounters an error it can't correct deep in multiple function calls. The exit() function should only be used in stand alone programs, never in operating systems code. When using the exit() function, use the macros EXIT_SUCCESS and EXIT_FAILURE that are defined in stdlib.h.

Don't Repeat Yourself
When code is repeating itself, it is better to write another function rather than repeating the code. Then the code only needs to be written and debugged once rather than multiple times. This is know as the DRY principlein software engineering.

Example: The following code has loops that repeats that

Decrease Function Complexity
Another software engineering principle is the Single Responsibility Principle. A function should only be responsible for one action, this makes each function easier to read, write, debug and use multiple times. It is much better to write smaller more concise functions() so that the can be used in multiple places and only need to be debugged once. The functions codeCheck() and main() would both benefit from applying this principle.

General Comments
The program should guide the user better. It's not clear what the input should be, there should be prompts for each input. The program might be more fun if the user could enter colors rather than numbers.

##The Good The choice of variable and function names is very descriptive. The code is properly indented. The consistent use of camelCase is excellent.

##MAGIC NUMBERS The term Magic Numbers refers to numerical constants in the code. A good programming practice is to used named constants rather than numbers. Named constants make the code more self documenting, and allow easier modification of the code. When a named constant is used, the code only needs to be changed in one location rather than multiple locations. An example of this in the code would be to increase or decrease the size of the arrays.

Note: The code above would be more efficient if you had an array of color strings and indexed into the the array using randColor as follows:

Not only does it make the function shorter, but indexing into the array is faster than the switch statement.

@MegaTom is correct; this code would be better using enums or named constants. The string compares are much less efficient that integer compares.

##Multiple Statements on a Line
To make future modifications easier, there should never be multiple statements on a single line. Let's say some code needed to be added to each case in the switch statement. Each of the cases in the switch statement would then be need to be broken into multiple lines which makes the edit more complex, and can create typos during the edit. It is much easier to just add another statement by adding a single line, rather than trying to add it to a single line.

##Prefer return over exit()
In the case of this program, since there is no error checking, there is no reason to use either exit() or return. If there is error handling in the program, the exit() function should only be used if the program encounters an error it can't correct deep in multiple function calls. The exit() function should only be used in standalone programs, never in operating systems code. When using the exit() function, use the macros EXIT_SUCCESS and EXIT_FAILURE that are defined in <stdlib.h>.

##Don't Repeat Yourself
When code is repeating itself, it is better to write another function rather than repeating the code. Then the code only needs to be written and debugged once rather than multiple times. This is known as the DRY principle in software engineering.

###Example: The following code has loops that repeat:

##Decrease Function Complexity
Another software engineering principle is the Single Responsibility Principle. A function should only be responsible for one action; this makes each function easier to read, write, debug and use multiple times. It is much better to write smaller more concise functions that can be used in multiple places and only need to be debugged once. The functions codeCheck() and main() would both benefit from applying this principle.

##General Comments
The program should guide the user better. It's not clear what the input should be; there should be prompts for each input. The program might be more fun if the user could enter colors rather than numbers.

Source Link
pacmaninbw
  • 26.1k
  • 13
  • 47
  • 114

The Good
The choice of variable and function names is very descriptive. The code is properly indented but. The consistent use of camelCode is excellent.

MAGIC NUMBERS
The term Magic Numbers refers to numerical constants in the code. A good programing practice is to used named constants rather than numbers. Named constants make the code more self documenting, and allow easier modification of the code. When a named constant is used the code only needs to be changed in one location rather than multiple locations. An example of this in the code would be to increase or decrease the size of the arrays.

Example Named Constants:

#define ARRAY_SIZE    4
#define STRING_SIZE   10
#define RED           1
#define YELLOW        2
#define GREEN         3
#define BLUE          4
#define BLACK         5
#define WHITE         6
#define MAX_GUESS     12
    

void makeCode(char secretCode[ARRAY_SIZE][STRING_SIZE])
{
    int i, randColor;
    for(i=0; i<ARRAY_SIZE; i++)
    {
        randColor = 1 + rand() % 6;     //creates a number
        switch(randColor)       //converts number created to a string
        {
            case RED:
                strcpy(secretCode[i], "red");
                break;
            case YELLOW:
                strcpy(secretCode[i], "yellow");       
                break;
            case GREEN:
                strcpy(secretCode[i], "green");       
                break;
            case BLUE:
                strcpy(secretCode[i], "blue");       
                break;
            case BLACK:
                strcpy(secretCode[i], "white");       
                break;
            case WHITE:
                strcpy(secretCode[i], "black");       
                break;
        }
    }
}

void displayGuess(char guessCode[ARRAY_SIZE][STRING_SIZE], int blackPeg, int whitePeg)
{
    int i;
    printf("\nYour Guess\t\t\t\tYour Score\n");
    for(i=0; i < ARRAY_SIZE; i++)
        printf("%s ", guessCode[i]);
    printf("\t\t");
    for(i=0; i<blackPeg; i++)
        printf("black ");
    for(i=0; i<whitePeg; i++)
        printf("white ");
    printf("\n\n");
}

Note: The code above would be more efficient if you had an array of color strings and indexed into the the array using randColor as follows:

void makeCode(char secretCode[ARRAY_SIZE][ARRAY_SIZE])
{
    int i, randColor;
    char *secretCodeColors[] =
        {
            "red",  // Multiple lines used to make it easier to add colors.
            "yellow",
            "green",
            "blue",
            "black",
            "white",
        };

    for(i=0; i<4; i++)
    {
        randColor = rand() % 6;
        strcpy(secretCode[i], secretCodeColors[randColor]);
    }
}

Not only does it make the function shorter, but indexing into the array is faster than the switch statement.

@MegaTom is correct, this code would be better using enums or named constants. The string compares are much less efficient that integer compares.

Multiple Statements on a Line
To make future modifications easier, there should never be multiple statements on a single line. Let's say some code needed to be added to each case in the switch statement. Each of the cases in the switch statement would then be need to be broken into multiple lines which makes the edit more complex, and can create typos during the edit. It is much easier to just add another statement by adding a single line, rather than trying to add it to a single line.

            case 1: strcpy(secretCode[i], "red");       break;
            case 1:
                strcpy(secretCode[i], "red");       
                break;

Generally it is better to assume that code will need to edited at some time in the future to add features or fix bugs.

Prefer return() over exit()
In the case of this program since there is no error checking there is no reason to use either exit() or return(). If there is error handling in the program, the exit() function should only be used if the program encounters an error it can't correct deep in multiple function calls. The exit() function should only be used in stand alone programs, never in operating systems code. When using the exit() function, use the macros EXIT_SUCCESS and EXIT_FAILURE that are defined in stdlib.h.

Don't Repeat Yourself
When code is repeating itself, it is better to write another function rather than repeating the code. Then the code only needs to be written and debugged once rather than multiple times. This is know as the DRY principlein software engineering.

Example: The following code has loops that repeats that

void displayGuess(char guessCode[ARRAY_SIZE][STRING_SIZE], int blackPeg, int whitePeg)
{
    int i;
    printf("\nYour Guess\t\t\t\tYour Score\n");
    for(i=0; i < ARRAY_SIZE; i++)
        printf("%s ", guessCode[i]);
    printf("\t\t");
    for(i=0; i < blackPeg; i++)
        printf("black ");
    for(i=0; i < whitePeg; i++)
        printf("white ");
    printf("\n\n");
}

void showPeg(int pegCount, char *colorString)
{
    for (i = 0; i < pegCount; i++)
        printf(colorString);
}

void displayGuess(char guessCode[ARRAY_SIZE][STRING_SIZE], int blackPeg, int whitePeg)
{
    int i;
    printf("\nYour Guess\t\t\t\tYour Score\n");
    for(i=0; i < ARRAY_SIZE; i++)
        printf("%s ", guessCode[i]);
    printf("\t\t");
    showPeg(blackPeg, "black ");
    showPeg(whitePeg, "white ");
    printf("\n\n");
}

Decrease Function Complexity
Another software engineering principle is the Single Responsibility Principle. A function should only be responsible for one action, this makes each function easier to read, write, debug and use multiple times. It is much better to write smaller more concise functions() so that the can be used in multiple places and only need to be debugged once. The functions codeCheck() and main() would both benefit from applying this principle.

General Comments
The program should guide the user better. It's not clear what the input should be, there should be prompts for each input. The program might be more fun if the user could enter colors rather than numbers.