Program that converts a number to a letter of the alphabet - follow-up

Question

This is a follow up from my previous post: Program that converts a number to a letter of the alphabet

This is meant to teach my little brother (who only knows how to make games in Geometry Dash) about C, and computers. This is an example I've written meant to be shown and explained to him.

#include <stdio.h>
#include <stdbool.h>

int main() {
  //A string is an array of characters
  const char * alphabet = "ABCDEFGHIJKLMNOPQRSTUVWSYZ";
  int temp,    letter   = 0;

  while( (1 <= letter && letter <= 26) == false){
    /* 
     * if scanf fails, valadate input to avoid infinite loop 
     * (the below while statement). 
     * Uncomment the below printf and comment out the below 
     * while statement to see what happens if you type 
     * something like "asdef4grvgrthd" instead of a number.
     */
    if(scanf("%d", &letter) != 1){
      while((temp=getchar()) != EOF && temp != '\n');
      //printf("Fun loop :) ");
    }
  }

  printf("The number %i corresponds to the letter '%c'\n", 
          letter, alphabet[letter-1]);
}

This is intended to help beginners understand C by being a slightly harder "Hello World" program.

Answer 1

• int letter is not a letter. When printing you call it The number. Name it accordingly: int number.

• The condition (1 <= letter && letter <= 26) == false is very hard to follow. As a general rule, avoid boolean constants in conditions. Rewriting it as:

  !(1 <= letter && letter <= 26)
  

  immediately calls for a deMorgan transformation into a much more readable form:

  (letter < 1 || letter > 26)
  

  It is also recommended to not rely on the operator precedence, which is very easy to get wrong. Use parenthesis instead:

  ((letter < 1) || (letter > 26))
  

• Avoid raw loops. Each loop represent an important algorithm, and as such deserves a name. The goal of:

  while((temp=getchar()) != EOF && temp != '\n');
  

  is to discard the line. Factor it out into a function:

  void discard_line()
  {
      int temp;
      while((temp=getchar()) != EOF && temp != '\n') {
      }
  }
  

  As a perk benefit, temp is no longer cluttering the essential logic.

  I also recommend to be a bit more explicit with the empty loops.

• Avoid magic numbers. 26 is really strlen(alphabet). Better yet, declare the alphabet as an array, rather than a pointer, and use sizeof.

Answer 2

Bug: When you feed an empty file to your program, it ends up in an endless loop.

Bug: the 24th letter of the English alphabet is X, not S.

Instead of const char * you should rather declare const char alphabet[], to make the code match the comment above it. Don't confuse strings and pointers to strings. The authors of the cs50 library do that, and they do much damage to the thousands of students who trust in them.

Answer 3

1. Never forget the trailing nul. sizeof(alphabet) is 27, not 26.

2. Don't use == false. It is too close to == true, which almost never works, especially in C. Use prefix operator ! instead. Then, as others suggest, consider reducing with de Morgan's laws, or maybe not.

3. (EOF & '\n' & '\r') evaluates to 8 (usually). This is not a set operation.

4. Consider using a do { ... } while (condition) loop.

Answer 4

Robust input is difficult to get right in C!

The input loop is broken when we reach end of file: it will loop indefinitely. What we want to do is to give up completely when we get EOF back from scanf():

  int items = scanf("%d", &letter);
  if (items == EOF) {
      fputs("Failed to read input\n", stderr);
      return EXIT_FAILURE;
  }
  if (items != 1) {
      /* skip the rest of this line, discarding the return value
         (we'll deal with errors next time round the loop). */
      scanf("%*[^\n]");
      continue;
  }

(We'll need to include <stdlib.h> for a definition of EXIT_FAILURE.)

---

The alphabetic characters can be declared static rather than auto. Although main() is only executed once in this program, the static declaration can help your compiler avoid unnecessary code.

---

The declaration of main() should be a prototype - explicitly show it takes no arguments by writing (void).

---

Modified program

This includes some observations mentioned in other reviews; I won't repeat them here.

#include <stdio.h>
#include <stdlib.h>

int main(void)
{
    static const char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    static const int length = sizeof alphabet - 1; /* don't count the NUL */

    int letter = 0;
    do {
        int items = scanf("%d", &letter);
        if (items == EOF) {
            fputs("Failed to read input\n", stderr);
            return EXIT_FAILURE;
        }
        if (items != 1) {
            /* skip the rest of this line, discarding the return value
               (we'll deal with errors next time round the loop). */
            scanf("%*[^\n]");
            continue;
        }
    } while (letter < 1 || letter > length);

    printf("The number %i corresponds to the letter '%c'\n",
           letter, alphabet[letter-1]);
}

Answer 5

Neither C nor C++ are the friendliest languages to get acquainted with programming, and character I/O is not their forte either. Besides, these raw C arrays/pointers are both an eyesore and a headache.

int temp,    letter   = 0;

I wouldn't mix uninitialized and initialized variables. For an educational code, each variable should be on a separate line with a comment defining its role.

This tinkering with EOFs and newlines is painful to watch. Handling errors is certainly a part of a programmer's job, but I would rather start with a program that can be read without prior knowledge of circa 1970 technicalities and introduce the gritty details only later.

I'd rather use getline() on stdin (letting the runtime handle this messy string allocation and terminators business) and atoi() to handle wrong input as simply as possible.
Or possibly use argc, argv to get rid of the I/O scan altogether, though the notion of command line arguments might not be terribly intuitive either at first glance.

Answer 6

A cardinal rule of good programming is to process return values appropriately.

Specifically, getchar() returns an int, not a success true/false boolean. Not processing the return value properly (here: while(!getchar())) leads to various undesirable effects, including the problem with the endless loop when fed an empty file.

Additionally, as numerous others have already pointed out, while( !(1 <= number && number < sizeof(alphabet)) ) is begging to be rewritten (not just for clarity, but for efficiency as well!) as while ((letter < 1) || (letter > sizeof(alphabet)))

And since you do not need to validate 'number' until after the number has been read. having two while loops is unnecessary.

If you combine (a) check return values, (b) validate inputs properly, you get something like this:

int number = 0;
while (1) {
     int c = getchar(); /* get something from stdin */
     if (c == EOF || c == '\r' || c == '\n') 
        break;  /* input completed */
     if (c < '0' || c > '9') 
        break; /* invalid input - treat as input completed */
     number = (number * 10) + (c - '0');
     if (number > 99) /* overflow? (more than two digits?) */ 
         number = 100; /* limit the overflow, but remember it */
}
if (number >= 1 && number < sizeof(alphabet))
    printf("The number %i corresponds to the letter '%c'\n", 
          number, alphabet[number-1]);