Asking for a name and showing the initials

Question

I've seen code that basically does the same thing, but i'd like some input on this. Basically what i did wrong or not good enough heh :).

#include <stdio.h>
#include <String.h>
#include <stdlib.h>
#include <ctype.h>
char *getName() {
    char name[100];
    int length;
    char *holder;
    printf("Tell me your name!\n");
    fgets(name,100,stdin);

    length = strlen(name)+1;

    holder = (char*)malloc(length*sizeof(char));
    strcpy(holder, name);

    printf("The name is: %s", holder);

    return holder;

}
char getInitials(char *name) {
    int length,i,j = 0;

    char *initials;


    length = strlen(name);

    initials = (char*)malloc(length*sizeof(char));

    for(i = 0;i < length;i++){
        if(i == 0)    {
            initials[j] = toupper(name[i]);
            j++;
      }
        else if(name[i] == ' '){
            initials[j] = toupper(name[i+1]);
            j++;

        }
    }
    initials[j] = '\0';
    printf("The initials are: %s",initials);
    free(initials);

}
int main() {
    char *name = getName();
    getInitials(name);
}

Answer 1

White Space

As was mentioned in the comments by @Aluan Haddad, your initial code was missing white space between things like operators and operands. As it stands, your code is still missing standard white space, such as a blank line between methods, a blank line between the end of your #include section and the start of the code. It makes your code feel a bit cramped.

Const

If you're not going to be modifying the contents of variables passed into functions you should be declaring the parameters as const. This helps the client to know that it is safe for example to pass a string literal into your getInitials function.

Users are unreliable

Even well users with good intentions make mistakes. You're not capturing all of those mistakes with your current code. What happens if the user puts two spaces between parts of their name?

Cleanup

In getName, you malloc up a buffer which you never clean up. It may be that you've intentionally done this because you know that it will be cleaned up anyway when the program exits. Alternately it may be that you've forgotten to do it. Generally I'd suggest always cleaning up after your malloc's because that way whoever is reading the code knows you've thought about it.

Naming

Some of your names are good and appropriate (name, initials), others are less descriptive (holder, j). Good naming makes your code a lot easier to follow.

Live up to your promises

getInitials says that it returns a char. It doesn't.

Be consistent

getName prompts the user, outputs what they tell the program, then returns the value. getInitials on the other hand parses a string, outputs the initials and then doesn't return anything. The prefix get really suggests that the methods should be doing a similar level of task.

Separate out user interactions

getInitials could be written in such a way that it took in a name and returned a string containing the initials, then it could be called from future programs that had different user interfaces. At the moment, it has a printf in it which makes this more challenging. Try to isolate your user interactions (inputs/outputs) from your main logic (string processing in this case).

Allocate how much?

getInitials allocates sufficient memory to store every character from the persons name. Is this really required? Since you've declared a buffer for the persons name as 100, should this really be a constant that can be reused so that getInitials doesn't need to malloc, it can just have a local buffer?

Multiple declarations on a single line

I don't really like them. I find that they make it hard to locate variables so I would avoid this:

int length,i,j = 0;

Unnecessary casting

You should avoid unnecessary casting. Since malloc returns a void*, you don't need to cast it when assigning it to a char*. You can simply do:

initials = malloc(length);

Answer 2

getName() review:

1. fgets(name,100,stdin) typically results in a '\n' in name. Lopping that usual optional '\n' right away makes sense.

2. fgets() may return NULL to indicate end-of file or error. Test for that.

3. Avoid magic numbers. Why 100 in fgets(name,100,stdin)? Better to code 100 only once.

4. Pedantic code does not rely on stdout being flushed with a '\n'. Add fflush(stdout) to insure prompt occurs before user input is requested.

5. length is a weak variable name choice. In C, the length of a string are the character count before the null character. Suggest size to indicate the +1

6. Casting the return value of malloc() is not needed in C. Multiplying by sizeof(char) is the same as multiplying by 1 -- not needed. If code wants to show that the allocation accounted for the size of the reference type, use sizeof *holder. Easy to code, review and maintain.

7. When printing a string, often useful to clearly indicate its ends, in case it begins/ends with white-space.

8. With user input of a name, I would trim leading/trailing whitespace right after fgets().

9. Check for successful malloc().

---

#define GETNAME_SIZE 100

char *getName() {
    // char name[100];
    char name[GETNAME_SIZE];

    int length;
    char *holder;
    printf("Tell me your name!\n");

    fflush(stdout); // add

    //fgets(name,100,stdin);
    if (fgets(name, sizeof name, stdin) == NULL) {      
      return NULL;
    }

    // add
    name[strcspn(name,"\n")] = '\0';
    // or
    trim(name);  // User code to trim leading/following white-space

    // length = strlen(name)+1;
    size = strlen(name)+1;

    // holder = (char*)malloc(length*sizeof(char));
    holder = malloc(size);
    // or
    holder = malloc(sizeof *holder * size);
    if (holder == NULL) {
      return NULL;
    }

    strcpy(holder, name);

    // printf("The name is: %s", holder);
    printf("The name is: <%s>\n", holder);

    return holder;
}