C function that creates a linked list with "divisible by 3" numbers from another linked list

Question

First, I need to create and show a list that ends with number 1000. That works well.

Then, I want to create another list with only the numbers that are divisible by 3 in the first list, but it doesn't work.

The worst thing is that it doesn't even tell me what's going on. It just gives error in the execution but the console doesn't say anything.

I will really appreciate any help.

I tried all.

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

#define CANTIDAD_NUMEROS 13
#define CANTIDAD_NUMEROS2 6
#define DESDE 1
#define HASTA 10

typedef struct lista{
    int num;
    struct lista *sig;
}nodo;


void crear (nodo *pt, int, int);
void crear2 (nodo *pt, int, nodo *pt2);
void mostrar(nodo *pt);

int main()
{
    int i=0;
    int t=0;

    nodo *prin;
    nodo *prin2;

    prin=(nodo*)malloc(sizeof(nodo));
    prin2=(nodo*)malloc(sizeof(nodo));

    crear(prin,i, t); //creates first list
    mostrar (prin); //shows first list
    crear2(prin,i, prin2); //gets 'divisible by 3' numbers
    mostrar(prin2); // shows second list

    return 0;
}

//creates list
void crear (nodo *registro, int cont, int t)
{

    scanf("%d", &t);
    registro->num = t;

    if (registro->num == 1000) 
        registro->sig=NULL;
    else
    {
        registro->sig=(nodo*)malloc(sizeof(nodo));
        cont++;
        crear (registro->sig,cont, t);
    }
    return;
}
//shows list
void mostrar (nodo *registro)
{
    if (registro->sig !=NULL)
    {
        printf ("%d\n",registro->num);
        mostrar (registro->sig);
    }else{
        printf("%d\n",registro->num);
    }
    return;
}
//creates second list with only numbers that are divisible by 3
void crear2 (nodo *registro, int cont, nodo *registroNuevo)
{
    if ((registro->num % 3) == 0){
        registroNuevo->num = registro->num;
        registroNuevo->sig = (nodo*)malloc(sizeof(nodo));
    }

    if(registro->sig != NULL){
        crear2(registro->sig,cont, registroNuevo->sig);
    }else{
        return;
    }

}

I expect to have the 1st list shown (which it's happening) and also the 2nd list shown with the numbers that are divisible by 3, which doesn't happen.

Just to clarify--this needs to be a linked list structure, not an array? — ggorlen, May 16 '19 at 23:10
Yes. It needs to be a linked list. The way I did it (but working)... — dragon25, May 16 '19 at 23:15
In the 1st list I stop the node creation when the user inserts 1000. — dragon25, May 16 '19 at 23:21
In the 2nd it is supposed to be created with the numbers that are divisible by 3 — dragon25, May 16 '19 at 23:21
Your creation functions are recursive for no reason; making those simple loops will simplify things and prevent stack problems. Also, in crear2(), if the first number is not a multiple of 3, then your recursive call includes registroNuevo->sig, which has never been set, so will invoke undefined behavior. In fact there's really no reason for rescursion anywhere here. — Lee Daniel Crocker, May 16 '19 at 23:59

ggorlen · Answer 1 · 2019-05-17T00:19:04.123

First of all, I admire your dedication to recursion!

The problem is that in crear2, registroNuevo->sig is uninitialized which causes a segfault. I almost always start a function that operates on a recursive linked data structure by checking if the parameter node is null. If so, I can safely continue on with the body of the function. Following this logic of protecting against nulls, we need to pass the registroNuevo node along without touching it in the case when registro->num % 3 != 0 and ensure all of its fields are initialized.

Here's the corrected function:

void crear2(nodo *registro, int cont, nodo *registroNuevo)
{
    if (registro) {
        if (registro->num % 3 == 0) {
            registroNuevo->num = registro->num;
            registroNuevo->sig = NULL;

            if (registro->sig) {
                registroNuevo->sig = malloc(sizeof(nodo));
            }

            crear2(registro->sig, cont, registroNuevo->sig);
        }
        else {
            crear2(registro->sig, cont, registroNuevo);
        }
    }
}

Having said that, this function is still a bit less than ideal for a couple reasons. First of all, the name is vague and could describe the behavior better. Also, if there are no items divisible by three, you've got a malloced node back in the calling scope that never gets initialized, so it's a bit brittle in that regard. Thirdly, even with a parameter, it feels like a highly specific function without much reusability factor that could be written iteratively inside the calling scope like:

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

typedef struct nodo
{
    int num;
    struct nodo *sig;
} nodo;

nodo *crear(nodo *registro, int num)
{
    nodo *n = malloc(sizeof(nodo));
    n->num = num;
    n->sig = registro;
    return n;
}

void mostrar(nodo *registro)
{
    if (registro)
    {
        printf("%d->", registro->num);
        mostrar(registro->sig);
    }
    else puts("");
}

void free_lista(nodo *registro) 
{
    if (registro) 
    {
        free_lista(registro->sig);
        free(registro);
    }
}

int main()
{
    nodo *prin = NULL;
    nodo *prin_div_3 = NULL;

    for (int t; scanf("%d", &t) && t != 1000;) 
    {
        prin = crear(prin, t);
    }

    nodo *tmp = prin;

    while (tmp)
    {
        if (tmp->num % 3 == 0) 
        {
            prin_div_3 = crear(prin_div_3, tmp->num);
        }

        tmp = tmp->sig;
    }

    mostrar(prin);
    mostrar(prin_div_3);
    free_lista(prin);
    free_lista(prin_div_3);
    return 0;
}

This isn't perfect--without tail nodes, adding to the list is a bit less than ideal, but dangling heads are eliminated, and hopefully it shows an alternate approach to organizing program logic and functions.

A few other remarks:

Always free memory that you've allocated. You can write a simple recursive routine to do so, like free_lista as shown in the above example.
Consider avoiding highly specific functions with hard-coded values like 3 and 1000. Make these parameters to maximize reusability.
crear2 never uses the cont member, and you have global constants that are unused. It's a good idea to clean these up to help clarify your debugging efforts and reduce visual clutter.
No need to cast the result of malloc.
if (registro->sig !=NULL) as the first line of a function is going to crash on a null. You don't need != NULL either. if (registro) { ... } is clearest and avoids problems with null parameters.

Thank you very much!!!! I'm trying to do what I mentioned in my other post, but I can't. — dragon25, May 17 '19 at 01:03
Do you think you can do another 2 functions that 1) add a 0 if the sum of the two numbers is more than 20. And 2) delete the second number if the sum of the two is less than 10. If you can I'll be very grateful. I'll show you here so you understand. https://imgur.com/a/mDOo4EX — dragon25, May 17 '19 at 01:13
I'm not entirely clear on what you're asking, but I'd encourage you to try it on your own--it sounds like a school assignment. — ggorlen, May 17 '19 at 01:17

score 1 · Answer 2 · answered May 17 '19 at 00:20

void crear2 (nodo *registro, int cont, nodo *registroNuevo) {

   if ((registro->num % 3) == 0) {
       registroNuevo->num = registro->num;
       registroNuevo->sig = (nodo*)malloc(sizeof(nodo));
       if (registro->sig != NULL)
           crear2(registro->sig, cont, registroNuevo->sig);
   }
   else {
       if (registro->sig != NULL)
           crear2(registro->sig, cont, registroNuevo);
   }
}

This is my approach, but you are still getting a final unexpected 0 at the last mostrar() call; and you still need to do the 'free' calls. I think you should avoid the recursive calls, there are easier ways to do it. Saludos.

C function that creates a linked list with "divisible by 3" numbers from another linked list

2 Answers2