return pointer from a C function is good/bad desig

Question

I want to know if return pointer from a C function is good/bad design? If it is a bad practice, what would be a good practice in the following example:

The question is a continued part of: c function return static variable

in data.h file:

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

typedef struct
{
   int age;
   int number;
} person;

person * getPersonInfo();

in data.c

#include "data.h"
static struct person* person_p = NULL;

person * getPersonInfo()
{
   person_p = (struct person*)malloc(10 * sizeof(struct person));
   return person_p;
}

in main.c

#include "data.h"

int main()
{
   person* pointer = getPersonInfo();
   return 0;
}

basically, the main function in main file needs to get the value of all the elements of array which is pointed by static pointer person_p, if it is not a good practice, then what a good practice should be?

Answer 1

The only reason it is bad is because you don't have any memory managing structure behind it. In your current code, you have a memory leak because you allocate a person struct via malloc() but do not free it.

Consider writing a wrapper function that handles that memory management for you like so:

void freePerson(struct person * personToDelete)
{
    free(personToDelete);
}

Then in your main:

int main()
{
   person* pointer = getPersonInfo();
   freePerson(pointer); // After you are done using it
   return 0;
}

I also have to warn against casting the results of malloc(). In my experience it can result in undefined behavior.

Answer 2

It's a matter of decision more than everything else.

Both options are valid as long as you're aware of each pro's and con's

Returning pointer pros:

1. Encapsulation of memory allocation
2. Support dynamic memory sizes on the fly

Returning pointer cons:

1. only one possible out parameter
2. Memory has to be allocated on the heap every time

Accepting pointer as input pros are exactly the opposite:

1. multiple out parameters
2. Memory does not have to be allocated on the heap, the caller can use a variable on it's stack to use as output parameter

Accepting pointer as input cons:

1. No encapsulation of memory allocation
2. No support for dynamic memory sizes on the fly - sizes must be fixed or pre-specified

In your specific example, I'd use an output parameter as it is fixed size and can easily be called without allocating memory dynamically.

Answer 3

To be a good design you must have a matching function that frees the allocated memory when it is no longer needed.

Answer 4

It is bad practice to return pointers to private variables. Also, with the current design, your .c file can only have one instance of the person object.

And almost needless to say, the same code which dynamically allocates an object should also be responsible of freeing it. Code which is written so that some other module in the program is expected to clean up the mess always ends up with memory leaks, per design.

If you are writing an at least somewhat complex data type, where you need to restrict access to private variables, have multiple instances of the struct etc, then it is good practice to use "opaque type" instead. Example (not tested):

// data.h

#ifndef DATA_H 
#define DATA_H

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

typedef struct person person; // opaque type

person* person_create (void);

void person_delete (person* p);

void person_set_age (person* p, int age);
int  person_get_age (const person* p);
// ... and so on, setters/getters

#endif // DATA_H

// data.c

#include "data.h"

struct
{
  int age;
  int number;
} person;    


person* person_create (void)
{
  return malloc(sizeof(struct person));
}

void person_delete (person* p)
{
  free(p);
}

void person_set_age (person* p, int age)
{
  p->age = age;
}

int person_get_age (const person* p)
{
  return p->age;
}

// caller:

#include "data.h"

int main()
{
   person* p = person_create();

   person_set_age(p, 50);
   printf("%d", person_get_age(p));

   person_delete(p);
   return 0;
}

Some things to consider:

• Always use header guards in h files.
• Never use empty parameter list () for C functions (but always do in C++).
• Don't cast the result of malloc in C (but always do in C++).
• There needs to be some error handling added to this code in case malloc fails.