6
\$\begingroup\$

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;
}
\$\endgroup\$
2

4 Answers 4

7
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Why recommend puts(), yet still use printf("Option: ");? Simply use fputs("Option: ", stdout);. IAC, best to use what is most clear to read. \$\endgroup\$ Aug 29, 2016 at 14:15
  • \$\begingroup\$ I totally forgot about this one. Thanks for reminding! I think it's better to leave printf() in that case to be as it doesn't force you to provide a specific stream, which is always stdout in this program. \$\endgroup\$
    – Aleks. M.
    Aug 29, 2016 at 14:24
  • \$\begingroup\$ If you're going to move the function prototypes to a header file, then there is no point in declaring the functions before the main function. Those can all be moved below main \$\endgroup\$
    – smac89
    Aug 29, 2016 at 14:55
  • \$\begingroup\$ Yes, I understand. That's why I mentioned it's not necessary, and that it's just my opinion that functions used in main() should be above main(). \$\endgroup\$
    – Aleks. M.
    Aug 29, 2016 at 15:05
  • \$\begingroup\$ @Aleks.M. Thank you for your post, it is tremendously helpful! So the main loop, or run loop should be part of the main function in a program such as this? Is there any advantages to this? Thank you! \$\endgroup\$
    – chris360
    Aug 29, 2016 at 20:49
5
\$\begingroup\$
  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");
    }   
    
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
-2
\$\begingroup\$

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().

\$\endgroup\$
3
  • \$\begingroup\$ 1) || option OFF certainly should be || option == OFF 2) This get_option() may be an infinite recursion should user enter a non-numeric. 3) Using option without qualifying the result of scanf_s() is not robust programming. 4) printf("Option: "); on buffered output lacks certainty it will display before user input is requested. Better to fflush(stdout); before seeking user input. 5) Inconsistent coding in initialize of float input_temp = 0;, yet not int option; \$\endgroup\$ Aug 29, 2016 at 16:52
  • \$\begingroup\$ Yeah, the == is indeed missing. Regarding the infinite recursioni am unsure what is better, exit the program or call again. Or would you count to N until you stop? Regarding the other stuff I am new here and dont know whether it is appropriate to just copy the original code. I wouldnt have written it that way on my own but in my opinion it is better to work with the provided code rather than complete new one?! \$\endgroup\$
    – miscco
    Aug 29, 2016 at 17:18
  • \$\begingroup\$ " unsure what is better, exit the program or call again. " -> Neither. Better to consume the offending non-numeric input then solicit input again or return with an indication to that effect. Should error/end-of-file occur, return a value to indicate that. "better to work with the provided code rather than complete new one?" --> When OP code is weak/faulty, better to use improved code. \$\endgroup\$ Aug 29, 2016 at 17:26

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.