patterncMinor
Asking for a name and showing the initials
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
Const
If you're not going to be modifying the contents of variables passed into functions you should be declaring the parameters as
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
Naming
Some of your names are good and appropriate (
Live up to your promises
Be consistent
Separate out user interactions
Allocate how much?
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:
Unnecessary casting
You should avoid unnecessary casting. Since
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.