0

Have a 

typedef struct person {
    char name[20]
    char surname[20]
} person_t;

I need to create a string like XXXXXX:YYYYYY with the function like char* personToString(person_t *p). I tried to make it:

char* personToString(person_t* p) {

  int n1,n2;
  n1=strlen(p->name);
  n2=strlen(p->surname);
  char *p = (char*) malloc((n1+n2+2)*sizeof(char));
  strcat(p,puser->name);
  strcat(p,":");
  strcat(p,puser->surname);

  return p;
}

This give me a reasonable output but I have some errors testing with valgrind! I also think that there is a way more classy to write the function!

Improve this question
7
  • 1
    Include the errors as they are pertinent to the question. Commented Jul 18, 2013 at 8:17
  • 3
    you can use sprintf instead of strcat, like sprintf(p, "%s:%s", p->name, p->surname);, plus u override p, u get it as parameter and then declare it as char * and you dont free your char * Commented Jul 18, 2013 at 8:19
  • sizeof(char) is not necessary since it's always 1 Commented Jul 18, 2013 at 8:21
  • Shouldn't it be struct person *p instead of person_t, since person_t is an object and not typedef? Commented Jul 18, 2013 at 8:24
  • i'm addinf also char ':' so i think i need +2! I tried with +1 and give me errors Commented Jul 18, 2013 at 8:27

4 Answers 4

Reset to default
3

When you malloc memory for p the memory will hold garbage values. Strcat will append a string after the null character, but in an uninitialized string will hold random values.

Replace the first strcat with strcpy.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

3

You need to

strcpy(p,puser->name);

not

strcat(p,puser->name);

malloc does not initialize the buffer to zero, so strcat is searching for a null byte in p first and probably not finding one, reading past the end of the buffer and thus crashing.

Instead of one strcpy plus two strcat you can also write one call to sprintf:

sprintf(p, "%s:%s", puser->name, puser->surname);
Improve this answer

3 Comments

you're right :D thank you so much! (st**id error for me) Even if it works well, do you think there's a way more classy to write the code?
Werner, You beaten us, your method is more appropriate to concatenate more then one strings. + to your answer. Nice
@user2590319 I'ld use sprintf. VoidPointer's implementation looks quite good to me.
2

First you should call string copy, then strcat: 

strcat(p,puser->name);

should be:

strcpy(p,puser->name);

because memory allocated with malloc function keeps values garbage, by doing strcat for first you are concatenating after garbage -- it also brings Undefined behaviour in your code.

You can use void* calloc (size_t num, size_t size); instead of malloc(), calloc function initialized allocated memory with 0 (then strcat() no problem). Also dynamically allocated memory you should deallocate memory block using void free (void* ptr);) explicitly.

Improve this answer

1 Comment

Just to add to the answer, Valgrind might be complaining about not freeing the memory
2

This looks good to me,

char* personToString( struct person_t *p )
{
  int len = strlen(p->name) + strlen(p->surname) + 2; // holds ':' + NULL 
  char *str = malloc( len ); // Never cast malloc's return value in C

  // Check str for NULL
  if( str == NULL )
  {
      // we are out of memory
      // handle errors
      return NULL;
  }

  snprintf( str, len, "%s:%s", p->name, p->surname);

  return str;
}

NOTE:

  1. Never cast malloc's return value in C.
  2. Use snprintf when multiple strcat is needed, its elegant.
  3. free the return value str here in caller.

Fixed struct and char variables.

Improve this answer

6 Comments

Shouldn't it be struct person *p instead of person_t, since person_t is an object and not typedef?
Nice answer actually I have suggestion change comment Never cast malloc's return value to Never cast malloc's return value in C
No need to snprintf, sprintf would suffice since you made sure the buffer is large enough. BTW, it's p, not puser. And there are too many closing braces in the snprintf line.
Checking p for NULL, than i might set value of errno? Like: if(p==NULL) { errno = EINVAL; return NULL; } is it good?
@WernerHenze snprintf() not needed here (because allocated correct) but it is preferable and safe.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.