0
\$\begingroup\$

Original code: Getting name from user, and handling any errors in C

I fixed the bugs. Is there anything else to consider?

#include "stdafx.h"
#include <stdio.h>
#include <string.h>
#include <ctype.h>
#include <stdbool.h>

#define BUFFSIZE 10

/*Checks to see if name contains actual characters and not just whitespace*/
bool hasAlpha(unsigned const char * name, size_t buffersize)
{
    for (size_t index = 0; index < buffersize && name[0] != '\0' ; ++index) {
        if (isalpha(name[index])) 
            return true;
    }

    return false;
}

/* @param[nlcspan], Used to determine if newline char was in C string or not */
/* @param[incomplete], If true, new line character is still in the buffer i.e. need to clear buffer*/


void retrieveName(unsigned char* name, size_t buffersize)
{
    for (;;) {
        if (fgets(name, buffersize, stdin) != NULL) {
            size_t nlcspn = strcspn(name, "\n");
            bool incomplete = name[nlcspn] == 0;
            name[nlcspn] = 0;
            if (incomplete) {
                int bufferclear;
                while ((bufferclear = getchar()) != '\n' && bufferclear != EOF);
            }
            if (name[0] == '\0' || !hasAlpha(name, buffersize)) {
                printf("Invalid name entered please try again...\n"
                       "USER NOTE: name cannot be empty and must contain at least one char\n");
            }
            else {
                return;
            }
        }
        else
            printf("Memory could not be allocated please try again...\n");
    }
}

int main(void)
{
    size_t len = BUFFSIZE;
    unsigned char name[BUFFSIZE];

    printf("Enter Name: ");
    retrieveName(name, len);
    printf("Your name is: %s\n", name);

    printf("Enter Name: ");
    retrieveName(name, len);
    printf("Your name is: %s\n", name);

    printf("Press enter to continue...\n");
    getchar();

    return 0;
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

BUGS
The function retrieveName() is doing the input of the name twice.

        if (fgets(name, buffersize, stdin) != NULL) {
            size_t nlcspn = strcspn(name, "\n");
            bool incomplete = name[nlcspn] == 0;
            name[nlcspn] = 0;
            if (incomplete) {                    int bufferclear;
                while ((bufferclear = getchar()) != '\n' && bufferclear != EOF);
            }
            if (name[0] == '\0' || !hasAlpha(name, buffersize)) {
                printf("Invalid name entered please try again...\n"
                       "USER NOTE: name cannot be empty and must contain at least one char\n");
            }
            else {
                return;
            }

Both fgets() and getchar() are getting input from stdin. The search you are performing for end of line is failing because bufferclear should be EOF on the first execution of the loop. What you should be doing is a search for end of line on name.

Keep it Simple
Since you are reading from stdin, use gets() rather than fgets(), gets() has the same semantics as fgets() but only reads from stdin. If you want retrieveName() to be able to read from any file you should add another parameter this is either the file name or a FILE pointer. If it is the file name than retrieveName() will need to perform a fopen().

There is a software engineering/programming principle call KISS, or Keep it Simple. _principle The simpler you make the code the easier it is to write, maintain and debug.

Alignment
If name is going to be used in a struct at some point in the future, you should consider the alignment of the fields in the struct to make the most efficient use of memory. Computers implement strings in words of memory. A word is some power of two bytes long, in the past this has been either 2 or 4 bytes, most computers and compilers are going to use 8 bytes these days in a 64 bit architecture. Therefore BUFFSIZE should be some multiple of 8 (8, 16, 24, 32). I'd suggest as a minimum 16 because 8 may be too small.

Use Functions or Loops for Repetitive Code In software engineering there is a programming principle called Don't Repeat Yourself. Any time you have repeating code such as :

    printf("Enter Name: ");
    retrieveName(name, len);
    printf("Your name is: %s\n", name);

in main() you should consider either creating a loop or writing a function. Generally I would recommend writing a function, but in this case a loop will do just as well.

\$\endgroup\$
5
  • \$\begingroup\$ Why should buffclear be an EOF at the beggining? Sorry my brains not working right now I just dont see why. Thank you for your answer! \$\endgroup\$
    – chris360
    Sep 13, 2016 at 18:54
  • \$\begingroup\$ Since you're only processing one line of input fgets() gets you to EOF on stdin. The call to fgets() and the getchar() while loop are equivalent. \$\endgroup\$
    – pacmaninbw
    Sep 13, 2016 at 19:05
  • \$\begingroup\$ Sorry to bother you after this length of time, I have been very busy and was really busy and unable to code all week. However I have a question regarding your first advice, the search of the end of the line is not failing because buffclear is not EOF on the first execution. It is the first character that was left over in the stream. bufferclear in this case then removes the unused input. Could you please clarify your first point? The program works exactly how it was intended only IF that while loop does not fail. If you take the time to read back over this I thank you! \$\endgroup\$
    – chris360
    Sep 17, 2016 at 1:09
  • \$\begingroup\$ Oh and also to further iterate that it is not failing, if you place a simple printf statement inside the while loop with perhaps an argument such as "1\n", and then type in anything more than 9 characters when running the program, lets say perhaps 15, there will be an output of 6 "1\n" ensuring that the loop has not failed and has indeed removed any leftover input from the buffer. Sorry for two lengthy comments I just want to learn and your first point doesn't quite make sense to me however all other points made are helpful/intuitive. \$\endgroup\$
    – chris360
    Sep 17, 2016 at 1:23
  • \$\begingroup\$ Omgosh I'm sorry, last thing, fgets() doesn't get me to an EOF on stdin, it gets me to BUFFSIZE - 1. Am I wrong? \$\endgroup\$
    – chris360
    Sep 17, 2016 at 1:40

Your Answer

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

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