Help! Error with simple list C++

When there is the case that the node is added before the first term works fine, but when I must add it after the first one continuously I get the error.

Seems like a perfect case for using a debugger step by step, it would be a good way to practice:)

The problem is localized, so let's debug the function insertar in second call for a greater N than the content in actual->edad:

void insertar(int N){
    nodo *nuevo=new nodo(N);nodo *aux=primero; actual=primero;
    bool k=0;
    int i=0;
    if (listaVacia()){
        primero=nuevo;
    }else{
        while(actual || k==0){
            if(actual->edad>N){
                nuevo->siguiente=actual;
                if (i==0) primero=nuevo;
                k=1;
            }else if (i==0){
                actual=actual->siguiente;
            }else{
                actual=actual->siguiente;
                aux=aux->siguiente;
            }
                i++;
        }
        if (k==0){
            actual=aux;
            actual->siguiente=nuevo;
        } 
    }
}

Assuming:

• (invented) address of primero: 0x00000001.
• primero->edad = 10.
• primero->siguiente = NULL.

We Call insertar(666):

1. initial values:
   • nuevo: 0x00000002 (invented address).
     • nuevo->edad = 666.
     • nuevo->siguiente = NULL.
   • aux: 0x00000001 (primero).
     • aux->edad = 10.
     • aux->siguiente = NULL.
   • actual: 0x00000001 (primero).
     • actual->edad = 10.
     • actual->siguiente = NULL.
   • k = false.
   • i = 0.
2. if (listaVacia()) is not met, we go to the branch else.
3. we check if we should start the loop while:
   1. actual is 0x00000001, which is considered true.
   2. the expression k == 0 is true.
   3. true or true is true: we start the loop.
4. we evaluate if(actual->edad>N):
   1. actual->edad is 10.
   2. N is 666.
   3. 10 > 666 is false: we go to the branch else.
5. we evaluate if (i==0):
   1. i is 0.
   2. 0 == 0 is true, we enter this branch.
6. we evaluate actual=actual->siguiente:
   1. actual is 0x0000001.
   2. actual->siguiente is NULL.
   3. actual is now NULL.
   4. we left the if current.
7. we evaluate i++:
   1. i is now 1.
   2. the return value is discarded.
8. we evaluate if (k==0):
   1. the expression k == 0 is true, we enter the if.
9. we evaluate actual=aux:
   1. actual is NULL.
   2. aux en 0x0000001 (primero).
   3. actual is now 0x0000001 (primero).
10. we evaluate actual->siguiente=nuevo:
    1. actual en 0x0000001 (primero).
    2. actual->siguiente is NULL.
    3. nuevo is 0x0000002.
    4. actual->siguiente is now 0x0000002.
    5. we exit the current if.
11. ends the loop, we re-evaluate the condition of the same actual || k==0:
    1. actual is 0x00000001, which is considered true.
    2. the expression k == 0 is true.
    3. true or true is true: we start the second turn of the loop.
12. we evaluate if(actual->edad>N):
    1. actual->edad is 10.
    2. N is 666.
    3. 10 > 666 is false: we go to the branch else.
13. we evaluate if (i==0):
    1. i is 1.
    2. 1 == 0 is false, we go to the branch else.
14. we evaluate actual=actual->siguiente:
    1. actual en 0x0000001 (primero).
    2. actual->siguiente is 0x0000002.
    3. actual is now 0x0000002.
15. we evaluate aux=aux->siguiente:
    1. aux en 0x0000001 (primero).
    2. aux->siguiente is NULL.
    3. aux is now NULL.
    4. we exit the current if.
16. we evaluate i++:
    1. i is now 2.
    2. the return value is discarded.
17. we evaluate if (k==0):
    1. the expression k == 0 is true, we enter the if.
18. we evaluate actual=aux:
    1. actual en 0x0000002 (nuevo).
    2. aux is NULL.
    3. actual is now NULL.
19. we evaluate actual->siguiente=nuevo:

In the step 19, you have skipped (with the arrow operator ->) from the memory address NULL, this causes your error as you cannot jump from NULL or de-reference NULL.

Your function insertar in addition to malfunctioning, it is too complex, check out this answer in which what you need is solved.

Well, in part Rene Garnica is somewhat right, your code is a bit complex for what it does, I've re-implemented a much simpler version of your code, without the need for circular references and those details.

Compared to your code, what you are looking for with the class - lista is to access the elements of this, which is almost unnecessary to have a Nodo only need its value and the next on the stack, so we implement this class Nodo:

class Nodo {
    public:
        Nodo *Siguiente;
        int Valor;
        Nodo(int);
        Nodo(Nodo*);
};

And its respective implementation:

// Implementación clase Nodo:
Nodo::Nodo(int a) {
    Valor = a;    // Se crea un nuevo nodo con un valor.
    Siguiente = NULL;
}
Nodo::Nodo(Nodo *n) {
    this->Valor = n->Valor;        // Se utiliza como constructor copia
    if (n->Siguiente != NULL)     
        this->Siguiente = n->Siguiente;
    else 
        this->Siguiente = NULL;
} 
// Fin implementación.  

As well as Class Lista:

class Lista {
    public:
        Lista();
        void Insertar(int);
        void Mostrar();
        Nodo *Nodos;
};

And its implementation:

//Implementacion clase Lista:
Lista::Lista() {
    Nodos = NULL;
}
void Lista::Insertar(int a) {
    if (Nodos == NULL) Nodos = new Nodo(a);
    else {
        Nodos->Siguiente = new Nodo(Nodos);
        Nodos->Valor = a;
    }
}

void Lista::Mostrar() {
    Nodo *Actual = new Nodo(Nodos);

    while (Actual != NULL) {
        cout << "Actual: " << Actual->Valor << '\n';
        if (Actual->Siguiente == NULL) break;
        else
            Actual = new Nodo(Actual->Siguiente);
    }
}

In this main():

#include <iostream>

using namespace std;

int main() {
    Lista *T = new Lista(); int a = 0, count = 0, max = 0;
    cout << "Inserte la cantidad maxima de elementos a introducir: "; cin >> max;
    while (count < max) {
        cout << "Escriba el elemento "<< count + 1 << " de " << max << ": "; cin >> a;
        T->Insertar(a); // Insertamos el elemento en la lista.
        ++count;
    }
    T->Mostrar();

    cin.get();
    return 0;
}

The problem with Segmentation Fault is because you are trying to access a memory address that is not being used or that has stopped being used, so we need to foresee this if we do not want problems when implementing our solution.

If you look at the constructor of Nodo, there are two constructors, the reason for this is that the memory addresses are not shared when using the pointer to the object, but a new instance is created and then reuse the existing ones.

I have tried this code and it gives me the following result:

Inserte la cantidad maxima de elementos a introducir: 10
Escriba el elemento 1 de 10: 1
Escriba el elemento 2 de 10: 2
Escriba el elemento 3 de 10: 3
Escriba el elemento 4 de 10: 4
Escriba el elemento 5 de 10: 523
Escriba el elemento 6 de 10: 23423
Escriba el elemento 7 de 10: 235
Escriba el elemento 8 de 10: 23
Escriba el elemento 9 de 10: 535
Escriba el elemento 10 de 10: 1234
Actual: 1234
Actual: 535
Actual: 23
Actual: 235
Actual: 23423
Actual: 523
Actual: 4
Actual: 3
Actual: 2
Actual: 1

Hello personally I think you have a bad design of the list as you were told it is not necessary to create circular references if you need to access the attributes of the Node class because leave them public or implement their chords getters but do not do more things.

Based on your code I have re-implemented the class ready to work personally I think it is more complicated to find the error than simply redoing the code adding that your implementation is something confusing.

Finally you lack to implement the destructor of the class since in no time you free the created nodes.

#include <iostream>

using namespace std;

class nodo
{
public:
    int edad;
    nodo *siguiente;

    nodo(int N)
    {
        edad=N;
        siguiente=NULL;
    };
};

class lista
{
private:
    nodo *cabeza;
public:

    lista (){
        cabeza=NULL;
    }

    //comprueba si la lista esta vacia
    bool listaVacia(){
        return (cabeza==NULL);
    }

    void insertar(int N){

        nodo *nuevo=new nodo(N);

        if(cabeza==NULL){
            cabeza=nuevo;
        }
        else{

            nodo *temp=cabeza;

            while(temp->siguiente)
            {
                temp=temp->siguiente;
            }
            temp->siguiente=nuevo;
        }
    }

    //Muestra los elementos de la lista
    void mostrar(){
        nodo *temp=cabeza;

        while(temp){
            cout<<temp->edad<<"--->";
            temp=temp->siguiente;
        }
        cout<<"NULL"<<endl;
    }


    //Destructor de la lista libera la memoria de los nodos
    ~lista(){

        while(cabeza){

            nodo *temp=cabeza;
            cabeza=cabeza->siguiente;
            delete temp;
        }
    }
};

int main()
{
    lista newlista;
    int A;

    for(int i=0; i<10; i++)
    {
        cout<<"Ingrese primera edad"<<endl;
        cin>>A;
        newlista.insertar(A);
        newlista.mostrar();
    }

    return 0;
}