HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Asking for a name and showing the initials

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theshowingandinitialsnameforasking

Problem

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 
#include 
#include 
#include 
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);
}

Solution

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);

Code Snippets

int length,i,j = 0;
initials = malloc(length);

Context

StackExchange Code Review Q#150227, answer score: 7

Revisions (0)

No revisions yet.