Temperature conversion in C

Question

I have started to learn C and I have tried my hand at temperature conversion. The idea is to convert a temperature from Fahrenheit to Celsius and vice/versa. Can somebody review my code and let me know how I might be able to improve it? My program is as follows:

#include <stdio.h>
void fahconv()
{
 double fah;
 puts("Enter value to convert from Fahrenheit to Celsius");
 scanf("%lf",&fah);
 printf("Converted temperature from F to C: %lf",((fah - 32.0)*5.0)/9.0);
}
void celsconv()
{
 double cels;
 puts("Enter value to convert from Celsius to Fahrenheit");
 scanf("%lf",&cels);
 printf("Converted temperature from C to F: %lf",(cels*9/5)+32);
}
void invalidchoice()
{
 printf("Invalid choice..exiting");
}
int main()
{
 char choice;
 puts("Enter (F or f) to convert from Fahrenheit to Celsius");
 puts("Enter (C or c) to convert from Celsius to Fahrenheit \n");
 scanf("%c",&choice);
 switch(choice)
 {
 case 'c': 
 case 'C': celsconv();
 break;
 case 'f': 
 case 'F': fahconv();
 break;
 default: invalidchoice();
 }
 return 0;
}

• Rob Starling: Considering your preference for wide open space for curly-brace-delimited things, your math (and comma) operators feel a bit snug. Regarding the actual math, your use of floats in fahconv vs ints in celsconv is distracting and worries that reader that it might be significant.
• snetch: Along with what everyone else said, I also notice you have added .0 to the numbers in one function but not in the other. Should probably add it to both.
• Malachi: can you perform mathematical operations between integers and doubles in C? the variable cels is declared a double.

Answer 1

• You don't need invalidchoice() as it just prints a statement and nothing else. Just put the printf() under default.

• Consider receiving all user input in main() and having the functions only do the calculations. It's best to have a function do one (useful) thing.

  In this form, the functions could also return the values to main() and displayed. With the user input eliminated, they should now take the original temperature as an argument.

• Optionally, you can return EXIT_SUCCESS or EXIT_FAILURE instead of 0 or 1, respectively. It's especially important to return a failure value if the conversion was not successful.

You could then have something like this:

#include <stdio.h>
#include <stdlib.h>
double fahconv(double fah)
{
 return (fah - 32) * 5 / 9;
}
double celsconv(double cels)
{
 return (cels * 9 / 5) + 32;
}
int main()
{
 double originalTemp;
 double convertedTemp;
 char choice;
 scanf("%lf", originalTemp);
 puts("Enter (F or f) to convert from Fahrenheit to Celsius\n");
 puts("Enter (C or c) to convert from Celsius to Fahrenheit\n");
 scanf("%c", &choice);
 switch (choice)
 {
 case 'c':
 case 'C':
 convertedTemp = celsconv(originalTemp);
 printf("Converted temperature from F to C: %lf", convertedTemp);
 break;
 case 'f': 
 case 'F':
 convertedTemp = fahconv(originalTemp);
 printf("Converted temperature from F to C: %lf", convertedTemp);
 break;
 default:
 printf("Invalid choice..exiting");
 return EXIT_FAILURE;
 }
 return EXIT_SUCCESS;
}

Answer 2

Jamal had a lot of good points, so just a few short things to add:

---

wordstogether is a confusing naming scheme. You should do one of the more standard naming schemes. In particular, either underscore_separator, or camelCase. It's also fairly common to use PascalCase for structs in C.

---

Rather than celsconv, I would state what the conversion is from and to. For example, fahrenheit_to_celsius and celsius_to_fahrenheit. Those make it immediately clear what the input is and what the output is. From the declaration, it's easy to see what they take, but their usage is more ambiguous (fahconv(deg) -- is deg Celsius? Kelvin?).

---

Since the performance of the switch is not a concern (it's not in a tight loop for example), I would be tempted to use tolower (from ctype.h) to simplify the switch to only have 1 case per character:

switch (tolower(choice)) {
 case 'c':
 /* celsius stuff */
 break;
 case 'f':
 /* ... */
 break;
 default:
 /* ... */
}

---

You should verify that all of the readings are successful. Otherwise, your later use of those variables are pretty dangerous/pointless. scanf returns the number of tokens successfully extracted, so the simplest way to verify it's success is to check number read == number expected.

Example:

double example;
if (scanf("%lf", &example) == 1) {
 /* success */
} else {
 /* failed :( */
}

Answer 3

My C is a bit rusty, but here's what I can do about it:

celsconv and fahconv seem redundant. You can simply ask the user the input value and in what way to convert it. Then in your switch, do the conversion. Afterwards output the result. The following is the shortest I could get it, minus some cosmetics.

double input;
char convertTo;
scanf("%f",&input);
scanf("%c",&convertTo);
switch(convertTo){
 case 'c':
 case 'C':
 printf("F to C: %f",(input - 32) * 5 / 9); 
 break;
 case 'f':
 case 'F': 
 printf("C to F: %f",(input * 9 / 5) + 32); 
 break;
 default: 
 printf("Invalid choice..exiting");
} 

• Yuushi: Perhaps in this simple case. However, getting into the habit of breaking things up into functions early is a good habit - personally I'd rather recommend leaving them as functions.
• Joseph: @Yuushi I forgot OP did say he was learning, so yeah, point taken. :D

Answer 4

Another choice is not to write an interactive program but to put the arguments on the command line. Many UNIX utilities take this simpler approach. Here is your program written that way:

#include <stdio.h>
#include <stdlib.h>
static double f_to_c(double fah)
{
 return (fah - 32) * 5 / 9;
}
static double c_to_f(double cels)
{
 return (cels * 9 / 5) + 32;
}
int main(int argc, char **argv)
{
 if (argc > 1) {
 double t = strtod(argv[1], 0);
 printf("%.2lfC = %.2lfF\n", t, c_to_f(t));
 printf("%.2lfF = %.2lfC\n", t, f_to_c(t));
 } else {
 printf("Usage: %s value\n", argv[0]);
 }
 return 0;
}

Notice that I have renamed your functions to short name equivalents of fahrenheit_to_celsius and celsius_to_fahrenheit suggested by @Corbin.

You may not have learned about command-line arguments yet. argc is the number of arguments on the command line including the command name itself. argv is an array of the nul-terminated strings, one for the command (argv[0]) and one for each argument you typed (argv[1] etc).

If you name the compiled program convert and run it, it will print the two possible conversions:

$ ./convert 32
32.00C = 89.60F
32.00F = 0.00C

• rolfl: For a commandline usage, I would recommend putting a loop on argc, and converting all the values presented.
• sc_ray: @William Morris - Thanks for this elegant snippet. I was wondering what is the purpose of having static before the functions?
• William Morris: Making functions static keeps their name local which avoids polluting the global namespace and makes some compiler optimizations possible. For a single file program that has no significance, but it is good practice when writing larger programs.

Answer 5

Another hint: Never use scanf to read numbers from a terminal. If the user types in a letter, the input gets "stuck" - this and anything later in the same scanf will not parse any input. (Read the spec for scanf to see what I mean.)

Best way to do this sort of thing:

1. Declare a buffer of size MAX_INPUT_LENGTH (of your choosing)
2. Use fgets to read a buffer of that size
3. Use sscanf to parse the number out.

The C library functions dealing with strings are a real disaster (as they can easily cause buffer overflow, among other problems). It's annoying that the safest way to do something as simple as what you're doing is as complex as this.