2
\$\begingroup\$

I am brand new to programming in C. I am following along with the book "The C Programming Language, Second Edition." I am making my own modifications to the example code presented in the book as I go along. Anyway I am posting my code here for review because I want to make sure I am off to a good start. There a few things in particular that I want to make sure that I am doing right as to not develop any future bad habits:

  1. I would like to make sure I am adhering to the C standards. Based on my research I found that the current and most recent standard for C is C17 or ISO/IEC 9899:2018, please correct me if I am wrong. As a result I have been compiling the code with the following parameters: gcc -Wall -pedantic-errors -std=c17 source.c Is this a good practice?

  2. I know I am a beginner but I would like to make sure that I am doing even the simplest things in a clean and efficient way.

So if I can get a review on this code sample, that would be great.

/* This program uses the formula C = (5/9)(F-32) to print a table of Farenheit
 * temperatures and their Centigrade or Celsius equivalents. Adapted from:
 * Kernighan, B. W., & Ritchie, D. M. (1988). The C programming language. */

#include <stdio.h>

/* Print Fahrenheit-Celsius table for Fahrenheit = 0, 20, ..., 300. */

int main(void)
{
    float fahr, celsius;
    int lower, upper, step;

    lower = 0;   /* Lower limit of temperature table. */
    upper = 300; /* Upper limit. */
    step = 20;   /* Step size. */

    printf("Fahrenheit to Celsius\n"); /* Table header */
    printf("%3s %6s\n", "F", "C");     /* Temperature label */

    fahr = lower;
    while (fahr <= upper) {
        celsius = (5.0/9.0) * (fahr-32.0);
        printf("%3.0f %6.1f\n", fahr, celsius);
        fahr = fahr + step;
    }
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I don't have a copy of K&R ready, but how much of the code provided is theirs and how much of it is yours? \$\endgroup\$
    – Mast
    Commented Aug 15, 2021 at 7:55
  • \$\begingroup\$ In a real application you'd generate all these values at compile time and store them in a read-only array. But if you are an absolute beginner, you'll need to study arrays and loops first :) \$\endgroup\$
    – Lundin
    Commented Aug 16, 2021 at 11:59
  • 2
    \$\begingroup\$ I have rolled back Rev 4 → 3. Please see What to do when someone answers. \$\endgroup\$ Commented Aug 16, 2021 at 17:58
  • 1
    \$\begingroup\$ @Lundin Really? I don't think it's possible to claim that a compile-time table is desirable over a simple loop in all cases. It depends on what you're optimizing for and what architecture you're on. The runtime-generated loop is much simpler and easier to maintain. \$\endgroup\$
    – Reinderien
    Commented Aug 16, 2021 at 18:03
  • \$\begingroup\$ @Reinderien There's always exceptions, but generally, on almost all systems from low end microcontrollers to high end 64 bit CPUs, execution time is much more valuable than read-only memory. \$\endgroup\$
    – Lundin
    Commented Aug 17, 2021 at 6:21

2 Answers 2

2
\$\begingroup\$
  • It's great that you're using C17.
  • Your gcc mostly looks sane, though I would add -ggdb and start learning about gdb.
  • Since you are using above-C99, I would rewrite
    float fahr, celsius;
    int lower, upper, step;

    lower = 0;   /* Lower limit of temperature table. */
    upper = 300; /* Upper limit. */
    step = 20;   /* Step size. */

as

    const int lower = 0,   // Lower limit of temperature table.
              upper = 300, // Upper limit.
              step = 20;   // Step size.

and move fahr to a for index:

for (double fahr = lower; fahr <= upper; fahr += step)

Also, due to the semantics of float promotion,

(5.0/9.0) * (fahr-32.0)

can just be

(fahr - 32)*5/9

This:

printf("Fahrenheit to Celsius\n"); /* Table header */
printf("%3s %6s\n", "F", "C");     /* Temperature label */

can use implicit string concatenation and character fields:

printf(
    "Fahrenheit to Celsius\n"  // Table header
    "%3c %6c\n",               // Temperature label 
    'F', 'C'
);
\$\endgroup\$
3
  • \$\begingroup\$ This was really great. Thank you. I am going to look into gdb, I ran the -ggdb flag but didn't notice anything different. \$\endgroup\$
    – drebrb
    Commented Aug 16, 2021 at 17:59
  • 1
    \$\begingroup\$ It's been rolled back - rather than doing that, if you want another round of review you should post a new question. \$\endgroup\$
    – Reinderien
    Commented Aug 16, 2021 at 18:00
  • 1
    \$\begingroup\$ Several declarations on a single line is generally considered bad practice/bad style since it increases the chance of bugs when using const, pointers etc. Mixing fixed point integer constants with floating point is considered very bad style since it's very easy to get integer promotion bugs. Your string concatenation example makes the code harder too read and there's no need for such pre-mature optimizations in a simple program like this. Overall, lots of bad and/or subjective advise, I have to down vote. \$\endgroup\$
    – Lundin
    Commented Aug 18, 2021 at 6:55
3
\$\begingroup\$

I want to make sure I am off to a good start.

float vs double

Save float for cases when serious reduced memory needed or when performance has compelled the use of narrow floating point, else use double as the go-to floating point type.

Units

300 what?
Physical units deserve clarity, perhaps a comment, in code. Be clear or oops!.

// upper = 300; /* Upper limit. */
upper = 300 /* F */; /* Upper limit. */

make sure I am adhering to the C standards.

Warnings

Using a modern compiler with many, if not all, warnings enabled is a good thing. Maybe add -Wetra.


make sure that I am doing even the simplest things in a clean and efficient way.

Overall

Code looks very clean and mostly efficient.

Unnecessary FP mixed operations

Notice by using float objects, yet double constants, code is like below incurring up and down FP conversions.

celsius = (float)((5.0/9.0) * ((double)fahr-32.0));
printf("%3.0f %6.1f\n", (double)fahr, (double)celsius);

If wanting to stay with float, use float constants

celsius = (5.0f/9.0f) * (fahr-32.0f);

Idea: abstract formatting

#define F_WIDTH1 3
#define C_WIDTH2 6

printf("%*s %*s\n", F_WIDTH, "F", C_WIDTH, "C");
...
    printf("%*.0f %*.1f\n", F_WIDTH, fahr, C_WIDTH, celsius);
\$\endgroup\$
7
  • \$\begingroup\$ This is great thank you. I am definitely going to implement and look further into your suggestions. I understand the float vs double suggestion but need to do further research into abstract formatting as I am not yet familiar with that one. \$\endgroup\$
    – drebrb
    Commented Aug 15, 2021 at 20:18
  • 1
    \$\begingroup\$ The float vs double constant problem is inherited from K&R. Your review makes several valid points but what you are actually reviewing is the verbatim of K&R. K&R apparently didn't know the difference between float and double either... it's such a harmful book. \$\endgroup\$
    – Lundin
    Commented Aug 18, 2021 at 6:58
  • \$\begingroup\$ @Lundin In K&R defense, at the time, all FP was typically very expensive vs. integer math. Somewhat true today, but often a a greatly lesser degree. ( I still use float primarily on embedded task lacking HW FP.) Benefits of float .v double were more significant. Agree K&R as a C primer today is at best: questionable. \$\endgroup\$ Commented Aug 18, 2021 at 12:43
  • 1
    \$\begingroup\$ @drebrb Although not a good primer, access to the C spec (or a draft of it) is very useful as it is definitive. \$\endgroup\$ Commented Aug 18, 2021 at 13:36
  • 1
    \$\begingroup\$ @drebrb Yes, Also see latest ANSI C and other via various web searches. \$\endgroup\$ Commented Aug 18, 2021 at 14:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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