4
\$\begingroup\$

I've just wrote my first real C program. It calculates the dimensions of a circle or a sphere based on the user input.

The program asks the user which dimension they are entering and then prints the values of the remaining dimensions.

#include <stdio.h>
#define _USE_MATH_DEFINES
#include<math.h>
#include<stdlib.h>

double radius, surfaceArea, area, volume, diameter, circumference;
char inputType[20];
char inputValue[30];
void printvalues(double radius);

int main()
{
    printf("Sphere and Circle Calculator\n");
    printf("Are you entering radius, diameter, circumference, area, surface area or volume? ");
    fgets(inputType, 20, stdin);

    if (_stricmp(inputType, "radius\n") == 0 || _stricmp(inputType, "r\n") == 0)
    {
        printf("Please enter the radius: ");
        fgets(inputValue, 20, stdin);
        radius = atof(inputValue);
        printvalues(radius);
    }
    else if (_stricmp(inputType, "diameter\n") == 0 || _stricmp(inputType, "d\n") == 0)
    {
        printf("Please enter the diameter: ");
        fgets(inputValue, 20, stdin);
        diameter = atof(inputValue);
        radius = diameter / 2;
        printvalues(radius);
    }
    else if (_stricmp(inputType, "circumference\n") == 0 || _stricmp(inputType, "c\n") == 0)
    {
        printf("Please enter the circumference: ");
        fgets(inputValue, 20, stdin);
        circumference = atof(inputValue);
        radius = (circumference/M_PI)/M_PI;
        printvalues(radius);
    }
    else if (_stricmp(inputType, "area\n") == 0 || _stricmp(inputType, "a\n") == 0)
    {
        printf("Please enter the 2D area: ");
        fgets(inputValue, 20, stdin);
        area = atof(inputValue);
        radius = sqrt(area / M_PI);
        printvalues(radius);
    }
    else if (_stricmp(inputType, "surface area\n") == 0 || _stricmp(inputType, "s\n") == 0 || _stricmp(inputType, "surface\n") == 0)
    {
        printf("Please enter the 3D surface area: ");
        fgets(inputValue, 20, stdin);
        surfaceArea = atof(inputValue); //r = √(A/(4π)) r=((V/π)(3/4))1/3
        radius = sqrt(surfaceArea / (4*M_PI)); 
        printvalues(radius);
    }
    else if (_stricmp(inputType, "volume\n") == 0 || _stricmp(inputType, "v\n") == 0 )
    {
        printf("Please enter the 3D surface area: ");
        fgets(inputValue, 20, stdin);
        volume = atof(inputValue); //r=((V/π)(3/4))1/3
        radius = cbrt(volume/M_PI)*0.75;
        printvalues(radius);
    }
    else if (_stricmp(inputType, "exit\n") == 0 || _stricmp(inputType, "q\n") == 0)
    {
        return 0;
    }
    else
    {
        printf("You did not enter a valid value\n");
    }

}

void printvalues(double radius)
{
    diameter = radius * 2;
    circumference = diameter * M_PI;
    area = M_PI * (radius * radius);
    volume = (4.0 / 3.0) * M_PI * pow(radius, 3);
    surfaceArea = 4.0 * M_PI * pow(radius, 2);
    printf("The radius is: %f \n", radius);
    printf("The diameter is: %f \n", diameter);
    printf("The circumference is: %f \n", circumference);
    printf("The 2D area is %f \n", area);
    printf("The 3D volume is: %f \n", volume);
    printf("The surface area is: %f \n", surfaceArea);
}

Everything seems to be working correctly but I'd like reviews so I can learn better C.

The next thing to do is make a loop so the user can make repeated calculations and to add code to let the user quit at anytime by typing exit.

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$
  1. Check your privilege.

    This is apparently Windows code. It won't compile on Linux. I added:

     #ifdef unix
         #include <string.h>
         #define _stricmp strcasecmp
     #endif
    
  2. Limit the scope of your symbols

    You have globals defined for radius, diameter, etc. But you do all your work inside main except for printing, which depends solely on radius that you pass as a parameter. So why not just make all the global variables locals?

    And you can make all the functions (except main) static as well, because you don't need to link against them. ;-)

  3. Avoid magic numbers.

    You use 20 and 30 for sizing two arrays. But you fall back to 20 in most cases. Use symbols for this kind of thing, or better still use an expression (ARRAY_SIZE) that will evaluate to the correct size. (FWIW, in this instance sizeof is the same as ARRAY_SIZE since it's a buffer of 8-bit chars. But as soon as you switch to wchar_t or something, all bets are off. Stick with ARRAY_SIZE.)

     char inputType[20];
     char inputValue[30];
    
     ...
    
         fgets(inputValue, 20, stdin);
    

    This could be changed to:

         fgets(inputValue, ARRAY_SIZE(inputValue), stdin);
    

    Or to:

     #define INPUT_BUFFER_MAX 20
     // or
     enum { INPUT_BUFFER_MAX=20 };
    
     char inputValue[INPUT_BUFFER_MAX];
    
         fgets(inputValue, INPUT_BUFFER_MAX, stdin);
    
  4. DRY (Don't Repeat Yourself)

    If you find yourself doing the same thing over and over again, stop.

    You do this repeatedly:

     if (_stricmp(inputType, "radius\n") == 0 || _stricmp(inputType, "r\n") == 0)
    

    You're making a couple of mistakes here. First, you're only allowing "predefined" abbreviations - why not just allow anything that matches ("radius" or "rad" or "radi" or "ra" or "r")? Next, trim off the newlines! You shouldn't be requiring whitespace as part of the match, only the keyword.

    Something like:

     if (strnicmp(inputType, strlen(inputType), "radius") == 0)
    

    But even that's a bit much to type, so maybe try something like:

     #define IS_TYPE(str) (strncasecmp(inputType, strlen(inputType), (str)) == 0)
    
     if (IS_TYPE("radius")) {
     }
     else if (IS_TYPE("circumference")) {
     }
     ...
    
  5. DRY, part 2

    Within each of those blocks, you are doing the same basic thing: print a type-specific prompt, get a string, convert it to float, apply a formula to extract the radius, print all the circle values using the radius.

    First, instead of printing the values using the radius in each block, why not move that to the end and only skip that step for invalid values?

    Next, extract the exact elements of the prompt that you need, and pass them to a general-purpose function to handle "print a prompt, get a string, convert it to float."

    Finally, figure out a way to generalize the conversion formula.

    So:

     enum { FROM_RADIUS, FROM_DIAMETER, FROM_CIRCUMFERENCE, ... };
    
     ...
     else if (IS_TYPE("diameter")) { conversion = FROM_DIAMETER; }
     else if (IS_TYPE("circumference")) { conversion = FROM_CIRCUMFERENCE; }
     ...
     else {
         fputs("Garbage in, garbage out!\n", stderr);
         exit(1);
     }
    
     float value = get_value(Prompt_strings[conversion]);
     print_values(conversion, value);
    
  6. Make your functions small. I mean small.

    This is a purely personal preference. I know there are coders out there comfortable writing 3000 line functions in C (and I hate them). But see how small your can get your functions. Ideally, see if you can get them down to < 10 lines. Not because of any magical significance to the number 10, but simply because you are trying hard to deal with one level of abstraction at a time.

    I know there are lots of counter-examples for this: Oh, but what if I'm writing an initialization function for my 50 element struct? What if I'm breaking a computation into simple steps?

    Obviously, don't enforce this rule at the expense of creating confusion. If a function's natural span is 20 lines, then write 20 lines. But try to only write those lines if you have to.

    In this case, you have a main that does application stuff. However, main is responsible for parsing command line arguments, which you have none, and determining the exit status (which you have none). I suggest you focus on that and move your application logic to a separate function:

     int 
     main(void) 
     {
         int result = circle_sphere_info();
         return result;
     }
    

    You might want to implement a loop that keeps reading and printing until the user exits on purpose. (q/exit) And that would be another function:

     main(void)
     {
         int result = circle_sphere_info_loop();
         return result;
     }
    
     int 
     circle_sphere_info_loop(void)
     {
         for (;;)
             if (!circle_sphere_info())
                 break;
    
         return 0; // CSI will just call exit() for a non-zero result   
     }
    

    I've already talked about minimizing the CSI function. But there's also the printvalues function you have that does two different things: it computes all the values for the circle/sphere based on the input radius, and it prints those values. That's one thing too many. Better to perform the computation in one function, fill out a struct, and perform the printing in a different function that takes the struct as a parameter.

    This will have the effect of creating lots of little functions that are each small enough to fit into a single StackOverflow window ;-) and that hopefully are parameterized and small enough to make writing unit tests really simple.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ It's not a good idea to define symbols beginning with _ - it's probably better to use strcasecmp in the code, and define that equal to _istrcmp on the non-POSIX platforms. (That said, identifiers beginning str are also reserved, so the fully conformant solution will involve a neutral name that can be used everywhere...) \$\endgroup\$ Oct 8, 2021 at 6:55
  • 4
    \$\begingroup\$ This is an overall good review but 4) is harmful advise. Don't invent strange function-like macros needlessly - either write a proper (inline) function or leave the code be. DRY should be applied with great care, it can be a very harmful thing when programmers decide to obfuscate their program to oblivion just to avoid repeating a few lines of code. KISS is always much more important than DRY. \$\endgroup\$
    – Lundin
    Oct 8, 2021 at 12:59
  • \$\begingroup\$ @Lundin I agree. Nothing is gained by hiding the local variable reference inside the macro. An inline function is exactly the way to do it. It doesn’t even need to be defined inline. The compiler will do what makes sense. \$\endgroup\$ Oct 12, 2021 at 15:42
3
\$\begingroup\$
  • It's bad practice to use non-standard functions needlessly. Instead of calling non-standard _stricmp all over the place, you can just iterate over the input string once and call tolower on every character. Then you can use standard strcmp instead. This should also improve performance ever so slightly.
  • Calling strcmp repeatedly like this is ok for a beginner-level program, but in a real program it is inefficient. For larger data in professional programs, you'd rather use a sorted table and binary search through it, or for very large data use a hash table.
  • Rather than comparing strings with a \n inside them, sanitize the input so that it doesn't contain any unwanted characters like that. Removing trailing newline character from fgets() input
  • There is no reason to declare all your variables outside main() - doing so is generally bad practice, though in case of single file projects it doesn't matter much.
  • Never use the ato... functions. The strto... functions are equivalent, except they also have error handling. In this case, replace atof with strtod(str,NULL).
\$\endgroup\$
2
  • \$\begingroup\$ "The strto... functions are equivalent" --> The standard does not have int strtoi() in lieu of int atoi(). Could roll your own. \$\endgroup\$ Oct 21, 2021 at 10:31
  • \$\begingroup\$ @chux-ReinstateMonica For the vast majority of cases, using long and optionally converting to int will do. On the only systems where int vs long actually matters for performance (very low-end microcontrollers), the programmer is assumed to be competent enough to write such a function manually. \$\endgroup\$
    – Lundin
    Oct 21, 2021 at 10:37
0
\$\begingroup\$

I am not experienced in C programming but there are some rules that everyone generally follows, aghast at the top has mentioned some. So following them I reiterated your code and here is what I came up with...

In the following code I have demonstrated the use of DRY - Don't Repeat Yourself and KISS - Keep it Stupid Simple.

How can you use these concepts?

Go through the code below, and you will find some changes. You will find that I reused your code with a few changes.

Why did I make those changes?

Firstly, in your code there is repetition of if and else conditions. You are checking for an entire string as well as the input type's first character. If you are checking for the first character of your input type then you really don't have to check for the entire string.

Secondly, your code is really difficult to read. You have done a great job of creating a separate function to print values, but you can do so much more.

Thirdly, you are heavily reliant on in-built functions of C. They are case sensitive and before you use them, you need to know whether or not you really need to use them.

Fourthly, global variables are bad and will cause you more problems then you can handle later along the lines. Don't use them unless absolutely necessary.

Finally, I did something funny with the pointers in the code below. I am using another function to calculate all the attributes of your circle then passing that information to the printer function. But before I do so, I am using malloc to allocate space for my array as the scope of the array is not limited to one function. Similarly, I am using function free inside the printer function to release the memory as it is no longer needed.

#include<stdio.h>
#include<stdlib.h>
#define _USE_MATH_DEFINES
#include<math.h>

double getRadiusFromDiameter(double diameter){
    return diameter / 2;
}

double getRadiusFromCircumference(double circumference){
    return circumference / (2 * M_PI);
}

double getRadiusFromArea(double area){
    return sqrt(area / M_PI);
}

double getRadiusFromSurfaceArea(double surfaceArea){
    return sqrt(surfaceArea / (4 * M_PI));
}

double getRadiusFromVolume(double volume){
    return  cbrt((volume / M_PI) * 0.75);
}

double *getCircleStats(double radius){
    double *stats = malloc(sizeof *stats * 6);
    stats[0] = radius; // radius - 0
    stats[1] = radius * 2; // diameter - 1
    stats[2] = 2 * radius * M_PI; // circumference - 2
    stats[3] = M_PI * (radius * radius); // area - 3
    stats[4] = (4.0 / 3.0) * M_PI * (radius * radius * radius); // volume - 4
    stats[5] = 4.0 * M_PI * (radius * radius); // surface area - 5
    return stats;
}

void printValues(double* stats){
    printf("The radius is: %f \n", stats[0]);
    printf("The diameter is: %f \n", stats[1]);
    printf("The circumference is: %f \n", stats[2]);
    printf("The 2D area is %f \n", stats[3]);
    printf("The 3D volume is: %f \n", stats[4]);
    printf("The surface area is: %f \n", stats[5]);
    free(stats);
}

double scanValue(){
    char value[20], *randomPointer;
    fflush(stdin); // Erase all the garbage from input buffer
    fgets(value, sizeof(value), stdin);
    return strtod(value, &randomPointer);
}

int main(){
    char inputType;

    printf("Sphere and Circle Calculator\n");
    printf("Are you entering radius, diameter, circumference, area, surface area or volume? ");
    inputType = fgetc(stdin);

    switch(inputType){
        case 'R':
        case 'r': printf("Please enter the radius: ");
                  printValues(getCircleStats(scanValue()));
                  break;

        case 'D':
        case 'd': printf("Please enter the diameter: ");
                  printValues(getCircleStats(getRadiusFromDiameter(scanValue())));
                  break;

        case 'C':
        case 'c': printf("Please enter the circumference: ");
                  printValues(getCircleStats(getRadiusFromCircumference(scanValue())));
                  break;

        case 'A':
        case 'a': printf("Please enter the 2D area: ");
                  printValues(getCircleStats(getRadiusFromArea(scanValue())));
                  break;

        case 'S':
        case 's': printf("Please enter the 3D area: ");
                  printValues(getCircleStats(getRadiusFromSurfaceArea(scanValue())));
                  break;

        case 'V':
        case 'v': printf("Please enter the volume: ");
                  printValues(getCircleStats(getRadiusFromVolume(scanValue())));
                  break;

        case 'E':
        case 'e':
        case 'Q':
        case 'q': printf("Exiting");
                  break;
        default:  printf("You did not enter a valid value\n");
                  break;
    }
    return 0;
}
```
\$\endgroup\$
7
  • \$\begingroup\$ Insufficient memory allocated. double *stats = malloc(6); --> double *stats = malloc(sizeof *stats * 6); \$\endgroup\$ Oct 21, 2021 at 10:18
  • \$\begingroup\$ fflush(stdin); is undefined behavior (UB). \$\endgroup\$ Oct 21, 2021 at 10:20
  • \$\begingroup\$ Better code checks the return of fgets(value, sizeof(value), stdin); \$\endgroup\$ Oct 21, 2021 at 10:21
  • \$\begingroup\$ fgetc(stdin); returns 257 different values. Saving in a char loses distinctiveness. Best to use int. \$\endgroup\$ Oct 21, 2021 at 10:23
  • \$\begingroup\$ Consider switch(inputType){ --> switch(tolower(inputType)){ rather than many paired cases. \$\endgroup\$ Oct 21, 2021 at 10:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.