Critique my program

This is a discussion on Critique my program within the C Programming forums, part of the General Programming Boards category; I'm trying to learn C on my own, at my own pace (as I never really grasped it while at ...

  1. #1
    Registered User
    Join Date
    Oct 2004
    Posts
    2

    Critique my program

    I'm trying to learn C on my own, at my own pace (as I never really grasped it while at college). I've been reading Ritchie & Kernighan's "The C Programming Language".

    I made a short program to convert a range of Fahrenheit temperatures to Celsius. It seems to compile and run OK using gcc in Linux. I'm wondering if someone could 'critique' my code, let me know if I am doing anything wrong. In particular, is it safe to convert an int to a float, like I did below, where function 'celsius' takes an int and returns a float to main?

    Code:
    /*a wee program to convert a range of Fahrenheit temperatures
    (-100 to 300 to be exact, in 20 degree increments) to Celsius*/
    
    #include <stdio.h>
    
    float celsius(int); /*function prototype*/
    
    main()
    {
        int f;
    
        for (f = -100; f <= 300; f = f + 20)
            printf("%d\t%.2f\n", f, celsius(f));
        return 0;
    }
    
    /*function celsius definition*/
    float celsius(int f)
    {
        float c;
    
        c = (5.0/9.0) * (f - 32.0);
        return c;
    }

  2. #2
    Gawking at stupidity
    Join Date
    Jul 2004
    Location
    Oregon, USA
    Posts
    3,166
    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.
    If you understand what you're doing, you're not learning anything.

  3. #3
    Code Goddess Prelude's Avatar
    Join Date
    Sep 2001
    Posts
    9,796
    >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);
    }
    My best code is written with the delete key.

  4. #4
    Registered User
    Join Date
    Oct 2004
    Posts
    2
    Quote Originally Posted by Prelude
    >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:
    Thanks for the advice. When I compiled my program with GCC it didn't give me any warnings about the float/double thing, but when I compiled the same program in Visual Studio I got:

    Code:
    f:\miscfiles\temp.c(22) : warning C4244: '=' : conversion from 'double ' to 'float ', possible loss of data

  5. #5
    ---
    Join Date
    May 2004
    Posts
    1,379
    with gcc you need to turn all the warnings on

  6. #6
    Gawking at stupidity
    Join Date
    Jul 2004
    Location
    Oregon, USA
    Posts
    3,166
    Quote 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.
    If you understand what you're doing, you're not learning anything.

  7. #7
    ---
    Join Date
    May 2004
    Posts
    1,379

    well i got no idea then

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 4
    Last Post: 02-21-2008, 09:39 AM
  2. Using variables in system()
    By Afro in forum C Programming
    Replies: 8
    Last Post: 07-03-2007, 12:27 PM
  3. Replies: 3
    Last Post: 03-04-2005, 01:46 PM
  4. My program, anyhelp
    By @licomb in forum C Programming
    Replies: 14
    Last Post: 08-14-2001, 10:04 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21