Explore our sites

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 10 down vote favorite

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;
}
share|improve this question
 
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. –  Rob Starling 21 hours ago
add comment

4 Answers

up vote 12 down vote accepted

  • 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.

You could then have something like this:

#include <stdio.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");
    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 0;
}
share|improve this answer
add comment
up vote 14 down vote

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 :( */
}
share|improve this answer
add comment
up vote 7 down vote

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");
}
share|improve this answer
8  
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. –  Yuushi yesterday
 
@Yuushi I forgot OP did say he was learning, so yeah, point taken. :D –  Joseph the Dreamer yesterday
add comment
up vote 3 down vote

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.

share|improve this answer
 
It does not get stuck – it does what you tell it to do. If you say scanf("%f", &f) you say read a float. What should it do if a float is not input? Discard it? That would be worse in many an occasion. That is not how the function works – nor how it is defined or documented. It is also very easy to check if input is correct. Problem is that people to often does not read the documentation. If one use it correctly there is no problem. Same goes for buffer overflow on string input. There are good way to use it correctly. It has all the options needed. –  Sukminder 15 hours ago
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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