Thread: Critique my program

It looks good to me.

Regarding the celcius() function -- the rule in C is that the int is promoted to a float (or double) if either operand of the operator is a float (or double). It works okay for you.

If you had:

Code:

{
  int a = 1, b = 2;
  float c;

  c = a/b;
}

You would expect c to be 0.5, but it's not. It's 0 because a and b are both ints so it's doing integer math instead of floating point. But since you're doing f - 32.0 you're fine.

>I'm wondering if someone could 'critique' my code, let me know if I am doing anything wrong.
I'd be happy to give you a quickie code review.

>float celsius(int); /*function prototype*/
The comment is redundant. The only thing this could be is a function prototype, so you can safely remove it. Concerning the function declaration, it's a good idea to include parameter names in the declaration as documentation. Also, when using floating-point data types, the default choice is double for many valid reasons. Reading this, I wondered why you chose to use float.

>main()
The latest C standard bans the implicit int that allows this function definition to work. It's best to be explicit at all times for return types and parameter lists. Concerning the empty parameter list, this is a tricky area of the language that can easily slip up the unwary. Because of that, it is highly recommended that you get into the habit of using void as the parameter list when no arguments are expected for a function.

>int f;
This name is too short. I can figure it out from context, but unless you're using very well known abbreviations, you should stick to informative names.

>for (f = -100; f <= 300; f = f + 20)
Nice use of whitespace to delimit tokens. A shorter and more conventional way of adding n to x is x += n though.

>return 0;
You realized that main returns int, very nice.

>/*function celsius definition*/
This comment serves no purpose. A better comment would describe what the function does briefly.

>float celsius(int f)
Once again, f is not a very informative name. I won't burn you on it because it's easy to figure out within context, so I'll just slap your wrist.

>float c;
double would be a better choice, see below

>c = (5.0/9.0) * (f - 32.0);
The type of the result of an expression is the type of the longest, floatiest operand. Because floating-point constants are double unless suffixed by f or F, your expression results in double and you try to assign it to a float. This warrants a warning because of assignment from a wider type to a narrower type. It would be better to just go with double from the get go, otherwise you need to do this:

Code:

c = (5.0f/9.0f) * (f - 32.0f);

But then you have so many f's that your parameter variable gets lost in the mix.

Concerning the celsius function, there's no need to declare c. You can simply return the expression and be done with it. Here are the suggested changes:

Code:

#include <stdio.h>

double celsius(int fahr);

int main(void)
{
    int fahr;

    for (fahr = -100; fahr <= 300; fahr += 20)
        printf("%d\t%.2f\n", fahr, celsius(fahr));

    return 0;
}

/* Convert Fahrenheit to Celsius */
double celsius(int fahr)
{
    return (5.0 / 9.0) * (fahr - 32.0);
}

Originally Posted by sand_man

with gcc you need to turn all the warnings on

Actually, even with all of the warnings turned on I don't get that warning with his code:

itsme@itsme:~/C$ gcc -Wall -ansi -pedantic celcius.c -o celcius
celcius.c:9: warning: return type defaults to `int'
itsme@itsme:~/C$

Unless I'm missing a compiler option that will generate more warnings.