Capitalised initials of username

Question

Given a list of words, this program outputs the starting letters of the words capitalized.

I was reviewing this question that solves the same task, but over time so many changes accumulated, that I think my programme bears little resemblance to the original one, so instead of posting it as an answer, I ask you for further improvement.

#include <stdio.h>
#include <ctype.h>
#include <stdbool.h>
#include <string.h>
#include <stdlib.h>

const int NAME_LEN = 160 + 1;

bool is_letter(char ch) 
{
    return ch >= 'a' && ch <= 'z';
}

bool is_terminating_char(char ch) 
{
    return ch == '\n' || ch == '\0';
}

const char * first_letters_only_capitalized(char words[NAME_LEN]) {

    char *capitals = malloc(sizeof(char) * NAME_LEN);
    int pos = -1;

    for(int i = 0; ! is_terminating_char(words[i]); i++) 
    {
        capitals[++pos] = is_letter(words[i]) ? toupper(words[i]) : words[i];

        while (is_letter(words[i])) 
        {
            i++;
        }
    }

    capitals[++pos] = '\0';

    return capitals;
}

int main() 
{
    char user_name[NAME_LEN], user_initials[NAME_LEN];

    puts("Enter your name, I will output the initials capitalized: ");
    if ( ! fgets(user_name, NAME_LEN, stdin))
    {
        puts("Error reading input.");
        return 1;
    }

    strcpy(user_initials, first_letters_only_capitalized(user_name));
    printf("%s\n", user_initials);
}

Example session:

Enter your name, I will output the initials capitalized: 
foo bar buz
FBB

Answer 1

Here are some things that may help you improve your code:

Avoid memory leaks

The first_letters_only_capitalized function allocates memory (or tries to, see next suggestion) and returns a pointer to that memory, but it is never freed by the calling function. Instead of using strcpy into a fixed size string, you could instead use the return value directly:

char *user_initials = first_letters_only_capitalized(user_name);
if (user_initials != NULL) {
    puts(user_initials);
}
free(user_initials);

Check for failed memory allocation

Whenever a program allocates memory explicitly, as with using malloc or calloc, it should check to make sure that the allocation succeeded before using the resulting pointer. When malloc fails, it returns NULL, so you could modify your program like this:

char *capitals = malloc(sizeof(char) * NAME_LEN);
if (capitals == NULL) {
    return capitals;
}

Use const appropriately

One of the functions in your program is declared like this:

const char * first_letters_only_capitalized(char words[NAME_LEN]) 

But I think it should be this instead:

char * first_letters_only_capitalized(const char words[NAME_LEN]) 

The reason is that the first signature says that that words might be altered but that the returned pointer may not be. That seems backwards from the real situation which is that words is not altered by the program (and therefore should be declared const) but that the return value may be used by the caller in any way, including by using free.

Use consistent formatting

Using consistent formatting helps readers of your code understand it without distraction. This code is mostly well formatted, but the opening brace of the first_letters_only_capitalized is on the same line, which is unlike all the other functions in the program.

Reconsider the sense of boolean functions

The only time is_termininating_char is used, its return value is negated. That suggests the function sense should be inverted:

bool is_not_terminating_char(char ch) 
{
    return ch != '\n' && ch != '\0';
}

Allow mixed case input

As the program stands, if the user enters "John Smith" the program returns "JOSM" which is rather odd. Instead, alter the program to allow for mixed case input. It may also be interesting to consider other cases such as hyphenated last names and last names such as "von Trapp" in which part of the name is never capitalized.

Trim leading space

The program currently does not trim non-alphabetic leading characters including spaces, so an input of " #!%!* Al Alpha" yields an output of " #!%!* ALAL".

Use standard functions where appropriate

Instead of writing your own, it would make more sense to me to use standard function isalpha rather than writing your own is_letter function.

Consider using pointers

Using pointers in your first_letters_only_capitalized function would make it more idiomatic C in my opinion. Your first_letters_only_capitalized function might look like this.

char * first_letters_only_capitalized(const char words[NAME_LEN]) 
{
    char *capitals = malloc(sizeof(char) * NAME_LEN);
    if (capitals == NULL) {
        return capitals;
    }
    char *init = capitals;

    for(const char *ch = words; is_not_terminating_char(*ch); ++ch)
    {
        if (isalpha(*ch)) {
            *init++ = toupper(*ch);
        }
        while (isalpha(*ch)) {
            ++ch;
        }
    }
    *init++ = '\0';

    return capitals;
}

Avoid overly long names

In writing this review, I typed first_letters_only_capitalized nine times. It's exhausting! Show some mercy to other programmers and use shorter names, although using a very long name for a function whose purpose is to shorten names might be amusing irony to some.

Answer 2

is_letter is a misleading name for a function that checks wether a character is a lower case letter.

But you don't need that function at all - there's no need to check if the charater is a lower case letter before calling toupper. It will return the character unchanged if it's not.

It's redundant to multiply by sizeof(char). It is defined in the standard as 1, and will always be. It just add visual noise to the code.

Answer 3

Memory management

@Edward pointed out that your malloc() was missing a corresponding free() — a memory leak. But I'd like to ask, why was malloc() used in the first place? In C, returning new strings to the caller is a memory-management headache, so functions are often designed to avoid having to do that altogether. A common convention is have the caller supply the buffer in which to put the results. This feels unnatural to programmers accustomed to higher-level languages, but memory management in C is such a headache that avoiding heap allocation is considered the lesser evil.

In this case, the result is guaranteed to be no longer than the input. That is a special case in which you can use an in-out parameter: just overwrite the input buffer with the results.

An additional design flaw is that NAME_LEN is baked into the parameter type. A good string-handling function should be prepared to accept a NUL-terminated string of any length. (I also consider words to be an unfortunate parameter name, since words[i] looks like it refers to the i^th word, when it actually refers to the i^th character of the name.)

Behaviour

Your function only works correctly with lowercase input. Even though the problem description lacks specificity, I'd consider this to be wrong behaviour. The way in which it fails is particularly interesting: "Barack Obama" produces "BAOB".

Another ambiguity in the spec is how to interpret characters that are neither letters nor spaces. Personally, I would expect "barack o'bama" to produce "BO", rather than "BOB" as your function does.

It would be nice if the function were robust to weird input. Ideally, "barackspacespaceobama" should produce "BO" instead of "B O".

Implementation details

Initializing pos = 0 would be more natural than pos = -1. Just replace all ++pos with pos++.

It's a good habit to make helper functions static (even though it makes no difference for a trivially small program like this). Personally, I wouldn't bother with defining an is_terminating_char() helper function.

It is unnecessary to test for is_letter() before calling toupper(). The documentation for toupper() guarantees that "All other arguments in the domain are returned unchanged."

The i++ of the for-loop header, while technically correct, is a bit misleading, since i can get incremented many times by the inner while loop.

Suggested solution

In another answer, I suggested that strpbrk() would be helpful. It turns out that strcspn() is more appropriate. One advantage of the solution below is that it is easy to redefine what constitutes a word. If you want to consider hyphens to be word delimiters, you can just add it to WORD_DELIMS.

Note that no special treatment of '\n' is necessary; it gets skipped over just like any other whitespace. In this solution, it turns out that you don't even need to explicitly NUL-terminate the output either – it gets copied automatically.

I've chosen to have the function return the length of the output string. We get that information for free, and there might be scenarios in which it could spare the caller from having to call strlen() on the result.

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

/**
 * Extracts the first character of each word of the name, converted to
 * uppercase, overwriting the name.  (Words are delimited by any amount
 * of whitespace.)  Returns the number of characters in the result
 * (i.e. the number of words in the original name).
 */
size_t initials(char *name) {
    static const char *WORD_DELIMS = "\t\n\v\f\r ";
    char *in = name, *out = name;

    // Skip any whitespace, then copy a character (which may be NUL)
    while ((*out = toupper(*(in += strspn(in, WORD_DELIMS))))) {
        in++; out++;
        // Skip over the rest of the word
        in += strcspn(in, WORD_DELIMS);
    }
    return out - name;
}

int main(int argc, char *argv[]) {
    char name[161];
    if (!fgets(name, sizeof(name), stdin)) {
        return EXIT_FAILURE;
    }
    initials(name);
    puts(name);
}

In case you find the loop to be too slick, here's an expanded version:

size_t initials(char *name) {
    static const char *WORD_DELIMS = "\t\n\v\f\r ";
    char *in = name, *out = name;
    do {
        in += strspn(in, WORD_DELIMS);     // Skip any whitespace
        *out = toupper(*in);               // Copy a character (maybe NUL)
        if (*in == '\0') {
            return out - name;
        }
        in++; out++;
        in += strcspn(in, WORD_DELIMS);    // Skip over the rest of the word
    } while (1);
}