5
\$\begingroup\$

This was an exercise from cs50. The goal was to get the initials in upper letter of a name that we got from user. So for "barack obama" we would get "BO", input like "abc" should output "A" and so on. I am interested in knowing what I can do to simplify it or be more efficient.

Do I need the check against '\0' in the while loop? I ask because the way I see it, when a user inputs a name, there will always be a '\n'(right ?), so it gets out of the while loop when its position has '\n' and then out of for loop so '\0' is never actually checked. Let me know if logic is right. I kept it there just in case.

#include <stdio.h>
#include <ctype.h>

#define NAME_LEN 80 + 1

int main(void)
{
    char userName[NAME_LEN], userInitials[NAME_LEN];
    int i, pos = -1;

    // ask user for a name
    fgets(userName, NAME_LEN, stdin);
    for(i = 0; userName[i] != '\n' && userName[i] != '\0'; i++)
    {
        if (userName[i] >= 'a' && userName[i] <= 'z')
        {
            userInitials[++pos] = userName[i] - ('a' - 'A');
        }
        else
        {
            userInitials[++pos] = userName[i];
        }
        while ((!isblank(userName[i])) && (userName[i] != '\n') && (userName[i] != '\0'))
        {
            i++;
        }
    }
    userInitials[++pos] = '\0';
    printf("%s\n", userInitials);

}
\$\endgroup\$
  • \$\begingroup\$ "Do i need the check against '\0' in the while loop?" -- Yes. "when a user inputs a name there will always be a '\n'(right ?)" -- Only when there is enough space. If the user types more than 79 characters for the fgets, there won't be a \n. \$\endgroup\$ – Spikatrix Jul 11 '15 at 9:28
  • \$\begingroup\$ I made so many changes that I though it would be appropriate to post another question: Capitalised initials of username \$\endgroup\$ – Caridorc Jul 11 '15 at 10:23
5
\$\begingroup\$

The compiler doubles as a code-analysis tool

Compiling your file like: gcc usename.c -Wall -pedantic Warns you of a bad practice in your code:

usename.c:12:5: warning: ignoring return value of ‘fgets’, declared with attribute warn_unused_result [-Wunused-result]
     fgets(userName, NAME_LEN, stdin);

You should use the return value to check if input reading happened correctly.

Small helper function to increase readibility

The line

userInitials[++pos] = userName[i] - ('a' - 'A');

is not so immediate to understand, I suggest writing a tiny function:

char to_uppercase(char letter) {
    return letter - ('a' - 'A');
}

and calling it like:

 userInitials[++pos] = to_uppercase(userName[i]);

The C99 Standard and variable scope

All major C compiler support C99, I suggest using the appropriate flag to enable it and write:

for(int i = 0; userName[i] != '\n' && userName[i] != '\0'; i++)

to shorten the scope of i.

Excellent parametrization

You defined a constant for NAME_LEN and named it meanigfully instead of sprinkling a random number all over your code, excellent!

For further improvement I suggest using a

static const int NAME_LEN = 80 + 1

For reasons discussed here.

\$\endgroup\$
3
\$\begingroup\$

Good for you, using fgets() instead of the insecure gets().

To answer your question, yes you need to check for the NUL terminator. You cannot rely on the newline being there, because:

  • The input might be longer than 80 characters, in which case the string will have been truncated with a NUL before the newline.
  • The input might not come from a user typing into your program. It could be any data piped to your program's stdin. For example, echo -n Barack Obama | print_initials in a Bash shell would feed a string to your print_initials program that contains no newline.

You could make more effective use of the standard C library. In particular, you might be interested in strpbrk() strspn() to find the next blank character and toupper() to convert to uppercase.1


1 I had originally recommended strpbrk(), but after having solved the function myself, I found that strspn() and strcspn() is more useful.

\$\endgroup\$
  • \$\begingroup\$ What does strpbrk stand for? \$\endgroup\$ – Caridorc Jul 11 '15 at 10:16
  • 1
    \$\begingroup\$ @Caridorc "Pointer to the next break" or something like that. Anyway it turns out that strspn() and strcspn() are more appropriate. \$\endgroup\$ – 200_success Jul 12 '15 at 4:37

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.