11
\$\begingroup\$

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.

\$\endgroup\$
  • 4
    \$\begingroup\$ C & C++ are two very different languages. Which is this supposed to be? It has aspects of both. Specifically, which compiler are you using. \$\endgroup\$ – AJNeufeld Jan 12 at 21:19
  • 4
    \$\begingroup\$ @AJNeufeld I tagged it in C++ as it works in both C && C++, although since this is supposed to be an example in C I've removed the C++ tag. \$\endgroup\$ – Alex Angel Jan 12 at 21:35
  • 2
    \$\begingroup\$ sizeof(alphabet) == 27, there's a trailing null character in there. strlen(alphabet) == 26. \$\endgroup\$ – cobbal Jan 13 at 0:13
  • 5
    \$\begingroup\$ Hi I have rolled back quite a few of your edits. It is important on this site to preserve valid answers and we ask that you not update your code to include feedback. Please see this meta post for more information on how you can share your improvements with the community. \$\endgroup\$ – brug Jan 13 at 0:52
19
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ Is "((letter < 1) || (letter > 26)" suppose to be "((letter < 1) || (letter > 26))" (extra parentheses messing)? \$\endgroup\$ – Alex Angel Jan 12 at 22:46
  • \$\begingroup\$ @AlexAngel Of course, thanks for noticing. \$\endgroup\$ – vnp Jan 12 at 23:04
  • 7
    \$\begingroup\$ The pattern min <= x && x <= max is pretty standard for a between test. I'd rather keep it that way instead of tearing the both number expressions apart. \$\endgroup\$ – Roland Illig Jan 12 at 23:05
  • \$\begingroup\$ You make good points -- renaming "letter" to number && declaring "alphabet" as an array. I'll edit the example. \$\endgroup\$ – Alex Angel Jan 12 at 23:15
  • 1
    \$\begingroup\$ Your de Morgan transformation is terrible. At least make it letter < 1 || 26 < letter (but I still prefer the "inside" formatting). I also find the additional parens make the code much less readable. \$\endgroup\$ – Martin Bonner supports Monica Jan 14 at 14:49
13
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Changed the extra 'S' to an 'X' and changed the variable "alphabet" to an array. I don't know how to fix the empty file bug. \$\endgroup\$ – Alex Angel Jan 12 at 23:26
  • \$\begingroup\$ @AlexAngel check for empty at the start and return null or 0. \$\endgroup\$ – Tom.Bowen89 Jan 14 at 8:03
2
\$\begingroup\$
  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.

\$\endgroup\$
  • \$\begingroup\$ 1) Fixed by changing "<= sizeof()" to "< sizeof()" \$\endgroup\$ – Alex Angel Jan 13 at 0:37
  • \$\begingroup\$ 2) Since most people are suggesting it I've made this change \$\endgroup\$ – Alex Angel Jan 13 at 0:42
  • \$\begingroup\$ 3) EOF (26) & '\n' (10) & '\r' (13) does equal 8, why is it not a set operation? \$\endgroup\$ – Alex Angel Jan 13 at 1:03
  • \$\begingroup\$ 1, 2 and 3 don't correspond to any of the code in the question. Are you sure that this is a review? \$\endgroup\$ – Toby Speight Jan 13 at 10:54
  • \$\begingroup\$ @TobySpeight: OP has been updating the source. \$\endgroup\$ – David G. Jan 13 at 12:30
2
\$\begingroup\$

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]);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ 1. You don't need to initialize letter because it is always initialized by scanf. 2. I would write the trailing while test as while (letter < 1 || length < letter) (it's much easier to read multiple comparisons of a single variable if they are all in the same direction). 3. I do miss BCPL's DO ... UNTIL which would have allowed until (1 <= letter && letter <= length). \$\endgroup\$ – Martin Bonner supports Monica Jan 14 at 14:55
1
\$\begingroup\$

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.

New contributor
kuroi neko is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
0
\$\begingroup\$

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]);
New contributor
user5438932 is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
  • 1
    \$\begingroup\$ Be careful - in the code posted alphabet is a pointer, so sizeof alphabet has no relation to the string length! \$\endgroup\$ – Toby Speight Jan 13 at 13:05
  • \$\begingroup\$ @user5438932: From what I can follow you check if the character == 'EOF', '\r', or '\n' --> if True check if the number is in the bounds of the alphabet length; else check if character is numeric --> if True then check in bounds; from there I can't tell what is happening or why the line number = (number * 10) + (c - '0'); and below is needed. \$\endgroup\$ – Alex Angel Jan 13 at 13:40
-1
\$\begingroup\$

This is the revised version

#include <stdio.h>

int main() {
  const char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
  int        number   = 0;

  while( !(1 <= number && number < sizeof(alphabet)) ){
    if(!scanf("%d", &number)){while(!getchar());}
  }

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

I have changed alphabet to an array instead of a pointer, replaced the extra 'S' in the alphabet variable to an 'X'. I also renamed letter to number and replaced the == false comparison to !(...). I have replaced scanf("%d", &number) != 1 with !scanf("%d", &number). Also removed stdbool.h.

I have been told that

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

In what case will NAND-ing all of them fail?

I have replaced this with!getchar()

Additionally:

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

I do not know how to fix this.

\$\endgroup\$
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ – Vogel612 Jan 15 at 11:28
-1
\$\begingroup\$

Revision #2

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

int main() {
  //A string is an array of characters
  static const char alphabet[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
  int               number     = 0;

//while(number is not between 1 to 26){get number/input}
  while( (number < 1) || (sizeof(alphabet) < number) ){
    const int c = scanf("%d", &number);
    switch(c){
      case 1:{
        break;
      }
      case EOF:{
        fputs("Failed to read input\n", stderr);
        return EXIT_FAILURE;
        break;
      }
      default:{
        scanf("%*[^\n\r]");
        break;
      }
    }
  }

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

Based on @TobySpeight revison and everyones suggestions.

\$\endgroup\$
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ – Vogel612 Jan 15 at 11:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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