Function to create single linked-list

Question

Given the following data struct:

typdef struct lista {
    int num1, num2;
    struct lista* sig;
} nodoNum ;

and a declared nodoNum pointer variable, the following function will take said variable and create nodes, link them and stop when the user inputs "0" as the first number and return a pointer to the first node (or a null pointer if numbers were provided).

nodoNum * crearLista(nodoNum * registro) {
    nodoNum * auxNodo;
    int tempNum1;
    printf("Introducir numero 1 >\n");
    scanf("%d", &tempNum1);
    if (tempNum1 != 0) {
        auxNodo = (nodoNum *) malloc(sizeof(nodoNum));
        auxNodo->num1 = tempNum1;
        printf("Introducir numero 2 >\n");
        scanf("%d", &auxNodo->num2);
        auxNodo->sig = crearLista(auxNodo);
        return auxNodo;
    } 
    else {
        return NULL;
    }
    
}

I have been asking some questions over at Stack Overflow to understand more about pointers. I have arrived at this solution after a while. I'm interested in knowing if I'm breaking some best practice or where there could be room for improvement. As far as I know, it works... but as a beginner, I can never be sure!

Answer 1

The obvious problems to me are:

1. You are not using your function's parameter.

2. You are using scanf.

3. You are using recursion where you should not use recursion.

4. You are mixing several different concerns (node creation, list organization, user input, input validation).

5. You are not handling errors.

Using your function's parameter

Since you don't use the parameter, we can ask, "what should the parameter be?"

This is a pitfall for linked lists. If your list uses a data structure to store list metadata, then the parameter would be a pointer to that data structure.

If the list just uses a single variable to store a pointer to the first node in the list, then your parameter should be a pointer to that variable. (So you can change the actual pointer to NULL, or replace NULL with a new pointer when you create the first node.)

Thus, your parameter would be something like:

... 
crearLista(nodoNum ** primer) 
{
    ...
}

And you would call it something like:

nodoNum * La_lista = NULL;

... main(...) {
    ... crearLista(&La_lista) ...
}

Using scanf.

Q: What is the difference between:

1. [[something cringe-worthy and incredibly painful]]; and
2. using scanf()?

A: If someone held a gun to your head, you would agree to #1.

The scanf function is very, very hard to use correctly. It is even harder to use effectively. The various ways it can fail are difficult for even experienced developers to understand and avoid. Did your pattern allow for white space? Will your pattern consume trailing newlines? What is the difference between " 1\n" and "1 \n" and "1\n"? The general answer to dealing with scanf is not to deal with scanf!

Here is a list of ways to avoid scanf, which you might want to bookmark

Recursion

Recursion can be a useful tool, which is why it is taught in even introductory programming courses. However, in C it is a tool that you must use judiciously.

Recursion implicitly uses the call stack to avoid looping. The cost of doing this is the creation of extra stack frames, which consume memory in the stack region.^[1]

This is important because the C stack is generally a limited resource. See this question & answer for more information.

What this means for you is that a linked list, which is normally an effectively-infinite data structure, will be constrained by the limits of the run-time stack. And, of course, when you reach those limits you will not encounter a recoverable error, but instead will suffer a stack overflow.

Instead of using recursion to input more nodes, use a looping control structure instead:

nodoNum ** registro; // parameter

// like this:
for (;;) {
    nodoNum * nn = create_nn_from_input(stdin);
    *registro = nn;
    registro = &nn->nn_sig;
}

// OR like this:
while (true) {
    nodoNum * nn = create_nn_from_input(stdin);
    *registro = nn;
    registro = &nn->nn_sig;
}

// OR like this:
nodoNum * nn = NULL;
do {
    nn = create_nn_from_input(stdin);

    if (not_null(nn)) {
        *registro = nn;
        registro = &nn->nn_sig;
    }
} while (not_null(nn));

Separation of concerns

In the object-oriented world this is sometimes referred to as the Single Responsibility Principle (or "SRP").

In essence, a function should have a single concern in order to clearly convey what it is doing, to avoid errors, and to be reusable.

Your function has to deal with multiple concerns, which creates confusion and introduces chances for errors to occur.

Consider reading one single number from the user. You are using scanf, which is not a great idea. In the event of non-numeric input, how do you recover from errors? (You don't.)

Now consider reading one node from the user. If the user enters zero (0), we will agree that is a valid sentinel value for "no node/end of input", so how would it work? What happens if the user enters a valid first value (non-zero), but then you see end-of-file for the second value?

Now consider what happens if you are unable to allocate memory for your next node. What happens?

In every case, the answer to "what happens if ... occurs" is going to be either "You have to restructure your code" or "You have to write more code".

When your different concerns are isolated in separate functions, both of those tasks are simple. When your different concerns are mixed together, they interfere with each other. It can be hard to reorganize your code when you have to think about the effects of changes on multiple different things. It can be really hard to add more code when you have to keep mixing in additional concerns.

I suggest that you identify the various things you are doing and create separate functions to perform each of them.

You may be alarmed to find that each of those functions will be small, and very simple. That is the objective!

A small, simple function will have few bugs. It will be easily understood. It can be easily re-used. These are all good things. ;-)

You may also find that "small simple functions" will tend to grow. When you realize that the concern of a function is doing exactly one simple thing, you may start thinking up "edge cases" to handle. Eventually you will have a very robust function that is not quite so small or simple, but which reliably does what it should do in even the most stressful conditions.

Handling errors

C is famous for its error handling. Not in a good way.

One of the challenges of being a C developer is knowing how to handle errors. When you start work on an existing project, there will be an existing error-handling mechanism that you will hopefully learn. When you are writing your own code de novo, you have to provide the error handling yourself.

For a simple command-line program, the easiest way to handle errors is to print some kind of message and then exit the program immediately. As your program becomes larger, you will want to allow the user to retry certain operations, or perform some kind of "graceful shutdown," or just abort whatever was in process and go back to an earlier state. These are all valid things to do, but C doesn't actually support you in any of them.

My suggestion would be to write yourself a failure function and a FAIL macro, and call it to handle errors for you:

noreturn void fail(const char * file, signed long line, const char * mesg, ...);
#define FAIL(...) fail(__FILE__, __LINE__, __VA_ARGS__)

The idea is that you make a decision about how failure is to be handled. Maybe it's just print-a-message-and-exit. (In fact, for beginner-level code that's always the right answer.) Then you implement that behavior inside fail(). Then you always call FAIL which will call fail on failure.

If you don't want to print-and-exit, there is one place to change the behavior. (Maybe you want to add logging. Maybe you want to longjmp to main and start over. Whatever, it's your code.)

Doing this let's you develop coding idioms that make sense for you. If you're going to call malloc, you'll want to check for errors. So...

if (malloc(size) == NULL) FAIL("Could not malloc(%u) memory for <reason>");

In fact, you might find yourself always writing your calls to malloc that way, in which case you can just create a new function for your personal library.

Obviously the same thing is true for fopen, strdup, etc. If all you do with errors is call FAIL, then write a wrapper function to simplify your code even more. If you do more than call FAIL, then ask yourself if you need to add lines to your fail function to simplify your code even more. ;-)

---

^[1] To truly be called recursion, the memory used must be from the Stack region. If the memory does not come from the Stack region, what you are doing is merely sparkling iteration.

Answer 2

There is a small typo in this line:

typdef struct lista {

It should be

typedef struct lista {

I ran sig through Google translate and it came up with Mr. Not very helpful, so I'm asuming the sig means next :-)

I used the following main function to confirm that it's working:

int main() {
    nodoNum* root = crearLista(0);
    for (nodoNum* nodo = root; nodo; nodo = nodo->sig)
        printf("%d, %d\n", nodo->num1, nodo->num2);
}

Recursion

Recursion is great when used in the correct context. Here it is not. Red flags go up when I see a recursive function without any depth indicator or checks. You can get a stack overflow exception if you piped enough numbers in.

Error checking

You should check if the scanf functions succeeded. If you enter a non-numeric value the program goes into a loop and crashes.

Additionally, you should check if malloc succeeded and exit the function gracefully if not.

Splitting out logic

You can create a separate function to add a node to the list and remove the complexities from crearLista. I would recommend something like this:

nodoNum* addNodo(nodoNum* registro, int num1, int num2) {
    nodoNum* auxNodo = (nodoNum*)malloc(sizeof(nodoNum));
    if (auxNodo == NULL)
        return NULL;
    auxNodo->num1 = num1;
    auxNodo->num2 = num2;
    if (registro != NULL)
        registro->sig = auxNodo;
    return auxNodo;
}