Simple temperature converter in C

Question

I have just begun to learn the C language and wanted to start off small with a temperature conversion calculator. The program works, and I'm looking for feedback on do's and don'ts as I am sure there are at least a few errors in technique that can be improved. (I just realized Fahrenheit is spelled incorrectly).

/*Calculates user provided temperature in Farenheight or Celsius depending on option chosen.*/

#include "stdafx.h" //don't actually know what this is for however will not compile without it.
#include <stdio.h>
#include <stdBool.h>
#include <conio.h>

/*Calculator options*/
#define option_1 1
#define option_2 2
#define option_3 3
#define option_4 4

void welcome();
void calculator();
void get_input(_Bool*, int*, float*);
void display_result(int, float);
float F_to_C(float);
float C_to_F(float);

int main(void)
{
    welcome();
    calculator();
    _getch();
}

void welcome() /*Welcome message + instruction*/
{
    printf("Welcome to the Temperaure Conversion Calculator\n\n");
    printf("Please select an option:\n\n");
    printf("Options:\t1)F to C\t2)C to F\t3)OFF\n\n");
}

void calculator() /*Conversion Calclator*/
{
    int option = 0;
    float input_temp = 0.0f;
    _Bool calc_off = 0;

    while (!calc_off)

    {
        get_input(&calc_off, &option, &input_temp);

        if (option == option_1) /*F to C*/
        {
            display_result(option_1, input_temp);
        }
        else if (option == option_2) /*C to F*/
        {
            display_result(option_2, input_temp);
        }
    }

}

void get_input(_Bool *calc_off, int *option, float *input_temp) /*Retrieve user input*/
{
    int trash = 0;
    int data_check;
    printf("Select option: "); scanf_s("%d", option); /*Obtains option*/
    scanf_s("%c", &trash); /*obtains newline character that will cause infinite loop*/

    while (*option != 1 && *option != 2 && *option != 3) /*incorrect option selection*/
    {
        display_result(option_4, option_4);
        scanf_s("%d", option);
        scanf_s("%c", &trash); /*Collects garbage newline character (Prevents infinite loop)*/

    }

    if (*option == option_3) /*Calculator off*/
    {
        *calc_off = 1;
        display_result(option_3, option_3);
        return;
    }

    printf("Temperature: "); scanf_s("%f", input_temp); /*Obtains temperature*/

    return;
}

void display_result(int option, float input_temp) /*Display results*/
{
    if (option == option_1)
        printf("Farenheight to Celsius of %.1f F is: %.1f C\n", input_temp, F_to_C(input_temp));

    else if (option == option_2)
        printf("Celsius to Farenheight of %.1f F is: %.1f F\n", input_temp, C_to_F(input_temp));

    else if (option == option_3)
        printf("Calculator OFF\n");

    else if (option == option_4)
        printf("Invalid option choice, please try again...\nOption: ");
}

float F_to_C(float input_temp) /*Convert Farenheight to Celsius*/
{
    return (5.0 / 9.0)*(input_temp - 32);
}

float C_to_F(float input_temp) /*Convert Celsius to Farenheight*/
{
    return (9.0 / 5.0)*(input_temp)+32;
}

Answer 1

Use an enum instead of #define. So instead of:

#define option_1 1
#define option_2 2
#define option_3 3
#define option_4 4

You can use enum:

enum {F_to_C, C_to_F, Off} option_type; // I am leaving off the fourth option!

You will also have to modify other portions of your code to use enums.

Stop using so many if statements. Use switch-case statements instead. There are a few places that you do this in your code but for example:

if (option == option_1)
    printf("Farenheight to Celsius of %.1f F is: %.1f C\n", input_temp, F_to_C(input_temp));

else if (option == option_2)
    printf("Celsius to Farenheight of %.1f F is: %.1f F\n", input_temp, C_to_F(input_temp));

else if (option == option_3)
    printf("Calculator OFF\n");

else if (option == option_4)
    printf("Invalid option choice, please try again...\nOption: ");

Instead:

switch(option) {
    case F_to_C:
        printf("Farenheight to Celsius of %.1f F is: %.1f C\n", input_temp, F_to_C(input_temp));
        break;
     ...
     default:
         printf("Invalid option choice, please try again...\nOption: ");
         break;

Notice that we have a default. You can give garbage as option_4, more than just the value 4. You should try handling this best you can.

Don't use _Bool! Just use bool and use true and false instead of 1 and 0. So this:

_Bool calc_off = 0;

Should be:

bool calc_off = false;

But actually I don't really believe you need calc_off because you use pointers unnecessarily.

This:

int option = 0;
float input_temp = 0.0f;
_Bool calc_off = 0;

while (!calc_off)

{
    get_input(&calc_off, &option, &input_temp);

    if (option == option_1) /*F to C*/ // Rewrite this to use switch case!
    {
        display_result(option_1, input_temp);
    }
    else if (option == option_2) /*C to F*/
    {
        display_result(option_2, input_temp);
    }
}

get_input in particular is troublesome.

Why not let get_input return an option? then:

while((option = get_input(...)) != Off) {

Answer 2

• It's <stdbool.h>, not <stdBool.h>.

• The need for "stdafx.h" is well understood as being used in Visual Studio, so there's no need for that comment.

• You could avoid the need for function prototypes by having main() at the very bottom.

  Also, this may not matter, but I sometimes like to order functions based on calling order. In this case, you can have display_result() last (or before main()) and welcome() first. This may also make it easier for you and even others to locate these functions.

• Remove that stray newline after the while keyword in calculator(). A good way to have clean code is to maintain consistency in even the smallest ways.

• You already have a void parameter in main(), so you should be consistent and add it to the other functions without parameters.

• Print statements that only print unformatted output, such as in your welcome() function, can use puts() instead of printf(). The former will also give you an extra newline, so stay with printf() if you don't want this for a certain statement.

• Don't use comments that state the obvious:

  /*Display results*/
  

  This applies to many of your comments, so remove them accordingly.

• This could be a bit confusing:

  printf("Temperature: "); scanf_s("%f", input_temp); /*Obtains temperature*/
  

  As mentioned in a previous answer, don't put multiple statements on one line. It's even worse here because these two statements are not alike, and the comment don't quite make sense here. Just put these on separate lines and remove the comment.

• There's no need to have a return at the very end of a void function. It doesn't really matter that there's already an early return in that function.

• The name calculator() is a noun, and it should be a verb since functions do stuff. You could also specify what is supposed to be calculated, no matter how obvious it may seem in this particular program.

• You can rename get_input() to something like retrieve_input() since people expect "get" functions ("getters") to simply return something.

Answer 3

If I had to guess, you're using visual studio to compile your source.

stdafx.h is typical name for a "precompiled header".

It can be helpful if there are a lot of large header files (usually from external libraries) that don't change frequently over time which you use in all of your compilation units (.c files) for reducing the time it takes to compile your project. For simple projects though, there isn't really a need.

You can turn off this feature by right clicking on your project and going to:

Properties -> Configuration Properties -> C/C++ -> Precompiled Headers -> Precompiled Header -> Not Using Precompiled Headers

Answer 4

In addition to the other answers some remarks:

• C is quite portable if you do not include platform-specific headers. Usually one can do without them. Is there really a reason to include conio.h and use getch() instead of standard getchar()? Also you do not need stdafx.h for such small projects -- and it's non-standard. One can workaround this but it's tiresome.

• scanf_s is an extension to C11 that's "only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT 1__ to the integer constant 1 before including <stdio.h>." (source)
  So I changed the code like this:

  #ifndef __STDC_LIB_EXT1__
  #pragma message "scanf_s not available!"
  #define scanf_s(...) scanf(__VA_ARGS__)
  #endif
  

• You use bool calc_off but set it to 0. Why that? Either use int or use bool but with true and false.

• Why use 3 print statements if the same can be achieved with one? For me this goes together as one thing that should be printed.

  printf("Welcome to the Temperaure Conversion Calculator\n\n"
         "Please select an option:\n\n"
         "Options:\t1)F to C\t2)C to F\t3)OFF\n\n");
  

• My compilers (gcc & clang) don't like this:

  return (5.0 / 9.0)*(input_temp - 32);
  

  5.0 and 9.0 are implicitly double. The conversion to float loses precision. This is not a big problem here, but it's just a thing to keep in mind. I prefer to shut these off (in this case) either by making these float variables, casting them or just going with double all the way.

• data_check is never used

Answer 5

A few things about style, based on my personal experience.

I don't like "sidebar" comments like:

x = 1;  // some comment

I replace those with:

// some comment
x = 1;

If the statement (e.g. x = 1) is wide, this burns a lot of horizontal space and the comment will probably exceed 80 columns.

Consider such a comment to be the "topic sentence" of the paragraph. When it's a sidebar, it's effectively like putting the topic sentence at the end of the paragraph.

Most style guides recommend keeping the width to 80 columns or less.

I prefer ANSI comments (e.g. // ...) over K&R (e.g. /* ... */) as they're much easier and faster to edit.

Try to avoid "if-else" ladders if possible. I replaced one of yours with a switch/case and the other, in the loop, using a continue

I prefer to put the return type of a function on a separate line [in column 1] and so the function will appear in column 1 on the next line. This makes it easy to find them with a regex like ^myfunc[(] which can make things easier when editing or analyzing source.

Never put multiple statements on a single line. This is very hard to see visually when looking at a lot of code:

x = 1; y = 2;

Always split them:

x = 1;
y = 2;

Personally, I prefer [as do several style guides] to not "burn" a line for a left brace:

if (...)
{

Rather, do:

if (...) {

---

Here is your code cleaned up for style as I've mentioned above. Also, look at some of the changes in vertical and horizontal whitespace:

// Calculates user provided temperature in Farenheight or Celsius depending on
// option chosen.

// don't actually know what this is for however will not compile without it.
#include "stdafx.h"
#include <stdio.h>
#include <stdBool.h>
#include <conio.h>

// Calculator options
#define option_1 1
#define option_2 2
#define option_3 3
#define option_4 4

void welcome();
void calculator();
void get_input(_Bool*, int*, float*);
void display_result(int, float);
float F_to_C(float);
float C_to_F(float);

int main(void)
{
    welcome();
    calculator();
    _getch();
}

// Welcome message + instruction
void welcome()
{
    printf("Welcome to the Temperaure Conversion Calculator\n\n");
    printf("Please select an option:\n\n");
    printf("Options:\t1)F to C\t2)C to F\t3)OFF\n\n");
}

// Conversion Calclator
void calculator()
{
    int option = 0;
    float input_temp = 0.0f;
    _Bool calc_off = 0;

    while (! calc_off) {
        get_input(&calc_off, &option, &input_temp);

        //F to C
        if (option == option_1) {
            display_result(option_1, input_temp);
            continue;
        }

        //C to F
        if (option == option_2) {
            display_result(option_2, input_temp);
            continue;
        }
    }
}

// Retrieve user input
void
get_input(_Bool *calc_off, int *option, float *input_temp)
{
    int trash = 0;
    int data_check;

    // Obtains option
    printf("Select option: ");
    scanf_s("%d", option);

    // obtains newline character that will cause infinite loop
    scanf_s("%c", &trash);

    // incorrect option selection
    while (*option != 1 && *option != 2 && *option != 3) {
        display_result(option_4, option_4);
        scanf_s("%d", option);
        //Collects garbage newline character (Prevents infinite loop)
        scanf_s("%c", &trash);
    }

    // Calculator off
    if (*option == option_3) {
        *calc_off = 1;
        display_result(option_3, option_3);
        return;
    }

    // Obtains temperature
    printf("Temperature: ");
    scanf_s("%f", input_temp);
}

// Display results
void
display_result(int option, float input_temp)
{

    switch (option) {
    case option_1:
        printf("Farenheight to Celsius of %.1f F is: %.1f C\n",
            input_temp, F_to_C(input_temp));
        break;

    case option_2:
        printf("Celsius to Farenheight of %.1f F is: %.1f F\n",
            input_temp, C_to_F(input_temp));
        break;

    case option_3:
        printf("Calculator OFF\n");
        break;

    case option_4:
        printf("Invalid option choice, please try again...\nOption: ");
        break;
    }
}

// Convert Farenheight to Celsius
float
F_to_C(float input_temp)
{
    return (5.0 / 9.0) * (input_temp - 32);
}

// Convert Celsius to Farenheight
float
C_to_F(float input_temp)
{
    return (9.0 / 5.0) * (input_temp) + 32;
}

Answer 6

One issue is that your function display_results is four functions in one.

The problem is that they are four unrelated functions — yes, they all have the job of displaying results, but:

• Everywhere in your program that display_results is called, the caller knows which of the four functions it wants to invoke
• There is no commonality between the four functions

thus you have gained nothing by combining the four functions into one function.

---

To fix, you need to break it apart into four functions; e.g.

void DisplayFtoC(float temp);
void DisplayCtoF(float temp);
void DisplayOff();
void DisplayInvalid();

and each time you want to display something, you invoke the appropriate function.

Answer 7

I would suggest using fgets() to read a full line of input, and use sscanf() (or strtol() or some) to parse it into a number. That works more intuitively with line buffered input, since pressing enter will return control to your program in any case (IIRC scanf("%d") will read past a newline waiting for the number.) Also the newline or anything else would not be left hanging in the input buffer, now you have an extra call to clean it up. (Though I haven't tested the behaviour on Windows in ages)

Some other things (or opinions)

• calc_off is a bit redundant, it's the same as returning option 3.
• I would separate getting the choice from the user and using it in logic to two functions. Now you have two places switching on the user's choice
• You're not checking the return value of scanf_s(). It's also not standard C.
• welcome and calculate are missing argument declarations.

Answer 8

Already many good comments are given. My few suggestions:

1. For functions without inputs, good practise is to use void.
2. I find if/else blocks inside a while loop unnecessary. You pass what is read and the display function has a decision path for options.
3. Good practice is to keep all macros in CAPITAL letters and functions in CaMel case to avoid confusion as the size of the program grows.
4. If a Boolean type is used, create an enum typedef of true and false and restrict input values.
5. For other integers, try to use unsigned as long as signed is not most important. When assigning 0, make it 0U to ensure the compiler and the underlying hardware base type don't play with it.

Answer 9

scanf_s("%c", &trash); is invalid code.

"%c" with scanf_s(), expects a size as well as a destination.

// C99 
scanf_s("%c", &trash, (rsize_t) sizeof trash);

// MS
// https://msdn.microsoft.com/en-us/library/w40768et.aspx 
scanf_s("%c", &trash, (unsigned) sizeof trash);