Simple Temperature Converter 2 in C

Question

I finally got around to redoing my Temperature conversion program. I incorporated many elements of everyone's comments, and am uploading the finished program. Any new, or problems that I missed? Maybe a few very late here where I am working. Main focus of this redo was keeping my functions short and single purposed.

#include <stdio.h>

void inter_face();
void welcome();
void option_prompt();
void input_temp_prompt();
int get_option();
float get_input_temp();
void display_F_to_C(float);
void display_C_to_F(float);
void display_off();
void display_error();
float F_to_C(float);
float C_to_F(float);

enum { F_TO_C = 1, C_TO_F = 2, OFF = 3 } option_type;


int main()
{
    inter_face();
    getchar();
}

void inter_face()
{
    int option = -1;
    float input_temp = 0.0;
    welcome();
    while (option != OFF) {

        option_prompt();
        option = get_option();

        if (option == F_TO_C || option == C_TO_F) {
            input_temp_prompt();
            input_temp = get_input_temp();
        }
        switch (option) {
        case F_TO_C:
            display_F_to_C(F_to_C(input_temp));
            break;
        case C_TO_F:
            display_C_to_F(C_to_F(input_temp));
            break;
        case OFF:
            display_off();
            break;
        default:
            display_error();
        }
     }
 }

 void welcome()
{
    printf("Temperature conversion Calculator!!\nPlease select an option:");
    printf("\n1.) F to C\t2.) C to F\t3.) off\n\n");
}   

void option_prompt()
{
    printf("Option: ");
}

void input_temp_prompt()
{
     printf("Temp: ");
}

float get_input_temp()
{
    float input_temp = 0;
    scanf_s("%f", &input_temp);
    return input_temp;
}

int get_option()
{
    int option;
    scanf_s("%d", &option);
    return option;
} 

void display_F_to_C(float converted_temp)
{
     printf("Celsius: %f\n", converted_temp);
}

void display_C_to_F(float converted_temp)
{
    printf("Fahrenheit: %f\n", converted_temp);
}

void display_off() 
{
    printf("OFF\n");
}

void display_error()
{
    printf("Incorrect input, try again\n");
}

float F_to_C(float input_temp) 
{
      return (5.0 / 9.0) * (input_temp - 32);
}

float C_to_F(float input_temp)
{
    return (9.0 / 5.0) * (input_temp)+32;
}

Answer 1

Firstly, I don't quite understand why are you declaring so much functions that consist of a single line dedicated to printing a string. They are only used once and in one file, so there's no need to use a function or constant. I suggest you to get rid of them and replace calls to these functions with their bodies. Also, put break; inside default case (Here you can learn more about the reason why you should do this):

    switch (option) {
        case F_TO_C:
            printf("Celsius: %f\n", F_to_C(input_temp));
            break;
        case C_TO_F:
            printf("Fahrenheit: %f\n", C_to_F(input_temp));
            break;
        case OFF:
            printf("OFF\n");
            break;
        default:
            printf("Incorrect input, try again\n");
            break;
    } 

Secondly, I recommend to use puts() instead of printf() when printing simple strings as it is obviously simpler and places a newline automatically (And you use it often throughout your code). Note that this doesn't involve cases when printing formatted strings.

    switch (option) {
        case F_TO_C:
            printf("Celsius: %f\n", F_to_C(input_temp));
            break;
        case C_TO_F:
            printf("Fahrenheit: %f\n", C_to_F(input_temp));
            break;
        case OFF:
            puts("OFF"); // No need for printf()!
            break;
        default:
            puts("Incorrect input, try again"); // No need for printf() as well!
            break;
    }

Thirdly, I'd suggest to move the body of inter_face() function to main() as it is not used anywhere else and basically creates a run loop, so I don't see any reasons why you should place it in a separate function. In addition to this, you probably want to define your functions before they're used, thus moving all functions above main(). It's not necessary as the compiler will find them anyway, yet I reckon the source code looks better that way (Just imagine reading a book from the end to the beginning).

float F_to_C(float input_temp) 
{
      return (5.0 / 9.0) * (input_temp - 32);
}

float C_to_F(float input_temp)
{
    return (9.0 / 5.0) * (input_temp)+32;
}

float get_input_temp()
{
    float input_temp = 0;
    scanf_s("%f", &input_temp);
    return input_temp;
}

int get_option()
{
    int option;
    scanf_s("%d", &option);
    return option;
} 

int main()
{
    int option = -1;
    float input_temp = 0.0;

    printf("Temperature conversion Calculator!!\nPlease select an option:");
    printf("\n1.) F to C\t2.) C to F\t3.) off\n\n");        

    while (option != OFF) {

        printf("Option: ");           
        option = get_option();

        if (option == F_TO_C || option == C_TO_F) {
            printf("Temp: ");   
            input_temp = get_input_temp();
        }

        switch (option) {
            case F_TO_C:
                printf("Celsius: %f\n", F_to_C(input_temp));
                break;
            case C_TO_F:
                printf("Fahrenheit: %f\n", C_to_F(input_temp));
                break;
            case OFF:
                puts("OFF"); // No need for printf()!
                break;
            default:
                puts("Incorrect input, try again"); // No need for printf() as well!
                break;
        }
    }

    getchar();
}

Fourthly, you should put function prototypes, enumerations and other objects that carry data (Of course, prototypes do not carry data, yet they belong in header files) into a separate header file (For instance, main.h). That way your source file looks a lot cleaner, and if someone wants to use these functions elsewhere, he can easily find this header file with needed prototypes within.

Fifthly, I'd consider either merging or getting rid of get_option() and get_input_temp() functions as they perform the same task, just with different data types. I suggest to create a single function called get_input() that returns a float (Since float can be compared to int without requiring you worry about loosing precision) instead if you don't want to put them inside main() as we did with other redundant functions before. You may then compare these variables normally as usual arithmetic conversions from int to float will occur when comparing float to int (You can learn more about such conversions here, here and here).

#include "main.h" // Prototypes and enum are there!    

float F_to_C(float input_temp) 
{
      return (5.0 / 9.0) * (input_temp - 32);
}

float C_to_F(float input_temp)
{
    return (9.0 / 5.0) * (input_temp)+32;
}

float get_input(void)
{
    float input = 0;
    scanf_s("%f", &input);
    return input;
}

int main()
{
    int option = -1;
    float input_temp = 0.0;

    printf("Temperature conversion Calculator!!\nPlease select an option:");
    printf("\n1.) F to C\t2.) C to F\t3.) off\n\n");        

    while (option != OFF) {

        printf("Option: ");           
        option = get_input();

        if (option == F_TO_C || option == C_TO_F) {
            printf("Temp: ");   
            input_temp = get_input();
        }

        switch (option) {
            case F_TO_C:
                printf("Celsius: %f\n", F_to_C(input_temp));
                break;
            case C_TO_F:
                printf("Fahrenheit: %f\n", C_to_F(input_temp));
                break;
            case OFF:
                puts("OFF"); // No need for printf()!
                break;
            default:
                puts("Incorrect input, try again"); // No need for printf() as well!
                break;
        }
    }

    getchar();
}

Also, be sure to check chux's answer. It contains even more useful tips and tricks to improve your code. Especially scanf_s() checks, option = -1 change and math simplification.

Answer 2

1. When declaring a function with no parameters, use (void). This will detect improper usage of the function. Without a function signature, the declaration does not indicate anything about the function arguments, just its return type. With a function definition, using int foo(void) { and int foo() { are the same. C99 Standard: 6.7.5.3 $10, $14

   // void welcome();
   void welcome(void);
   
   void inter_face(void) {
     welcome("Hello");  // compiler error.
   

2. Using F_TO_C as a function and and an enumeration is less clear than using distinctive identifiers.

3. Inconsistence use of constants F_TO_C, C_TO_F, OFF with -1. If option is to take on symbols, rather than integers, use symbols throughout. I'd expect an OPTION_INVALID.

   // int option = -1;
   int option = OPTION_INVALID
   

4. Not checking the return value of scanf() family of functions should fail any review. User input is evil - do not trust it. Better yet, use fgets(), then parse the buffer.

   // scanf_s("%f", &input_temp);
   if (scanf_s("%f", &input_temp) != 1) Handle_InputError();
   

5. Simplify calculation. Code uses float as variables but uses double math. Code converts float to double to do the multiplication and then narrows back to float.

   F_to_C(float input_temp) {
     //        double constant
     // return (5.0 / 9.0) * (input_temp - 32);
     // Just use float math.
     return (5.0f / 9.0f) * (input_temp - 32);
   

6. Unclear why float variables are used as they are best for space and sometimes speed and neither of those issues apply here. Recommend double throughout.

7. Consider splitting string literals for clarity

   printf("\n1.) F to C\t2.) C to F\t3.) off\n\n");
   // vs.   
   printf("\n" 
       "1.) F to C\t2.) C to F\t3.) off\n"
       "\n");
   

8. Minor. Use an automatic formatting tool. Such a tool would not make a trivial extra space here. Avoid manual formating.

   // Extra leading space.
    void welcome()
   {
       printf("Temperature conversion Calculator!!\nPlease select an option:");
       printf("\n1.) F to C\t2.) C to F\t3.) off\n\n");
   }   
   

Answer 3

Just my quick 2 cents:

• I'd move all the prototypes in an external file, something like temperature.h.

• You use option_prompt only to display the prompt and then read it, so you're probably better off removing option_prompt() and putting that printf inside of get_option(). The same goes for input_temp_prompt() and get_input_temp().

• I cannot test it right now, but what if the user inputs a letter, or something like 5/2 at any of the prompts? My guess is that scanf_s is safe only regarding memory allocation, I doubt there's any check on the type of value that you scan. To be safer, you should probably read input through something like gets or getc, check that it's something acceptable and only after that, assign it to a variable that you can then use.

Answer 4

Similar to ChatterOne, i would suggest to consolidate your functions:

int get_option()
{
    printf("Option: ");
    int option;
    scanf_s("%d", &option);
    return option;
} 

float get_input_temp()
{
    printf("Temp: ");
    float input_temp = 0;
    scanf_s("%f", &input_temp);
    return input_temp;
}

Also it is necessary to check whether the provided input is valid:

int get_option()
{
    printf("Option: ");
    int option;
    scanf_s("%d", &option);
    if (option == F_TO_C || option == C_TO_F || option OFF) {
        return option;
    } else {            
        printf("Incorrect input, try again\n");
        return get_option();
    }
} 

Note, that i have added an recursive call to get_option in the error case. Similar changes should apply for get_input_temp().