Linked list loads values sometimes

Question

Linked list loads values sometimes

Navigation

#1 by (1 votes)
#2 by (1 votes)

0

I'm doing a program with a menu to load, delete and display a linked list through the list and node class. The problem is that sometimes all the values are loaded normally, and sometimes the program crashes after loading one or two values. If I try to create a list and the charge is outside the do-while menu, the program only loads 17 elements. If I try to load one more, the program closes.

#include <iostream>
#include <string.h>
#include <cstring>
#include "Nodo.h"
#include "Stack.h"
#include "Lista.h"

using namespace std;

void menu(){
  cout<<"1: Ingresar cliente "<<endl;
  cout<<"2: Eliminar cliente "<<endl;
  cout<<"3: Visualizar clientes "<<endl;
  cout<<"4: Salir "<<endl;
}

int main(){
  Lista <string> l;
  string name;
  int op,pos;

  do{
    menu();
    cin >> op;

    switch(op){

      case 1:

        cout<<"Ingrese Nombre\n";
        cin>>name;
        l.insertValue(name);
        break;     

      case 2:

        cout<<"Ingrese cliente a eliminar\n";
        cin>>pos;
        l.deleteAt(pos);
        break;   

      case 3:

        l.printList();
    }

    system("pause");      
    system("cls");  

  }while(op!=4);

  Lista <string> l1;
  return 0;
}

List.h

    #include "Nodo.h"
    #include <iostream>
    #ifndef  _LISTA_H
    #define  _LISTA_H

    using namespace std;

    template <class T> class Lista{

      protected:

        T dato;
        Nodo<T>* lista;

      public:

        Lista(){
          lista=0;
        }

        void deleteAt(int p){
          Nodo<T>* temp = lista;

          if(p==1){
            lista=lista->obtenerEnlace();
            free(temp);
          }
          else{
            for(int i =0;i<p-2;i++) temp=temp->obtenerEnlace();

            Nodo<T>* temp2 =temp->obtenerEnlace();
            temp->ponerEnlace(temp2->obtenerEnlace());
            free(temp2);
          }
        }

        void insertValue(T d){
          Nodo<T> * nuevo =(Nodo<T>*)malloc(sizeof(Nodo<T>));
          nuevo->ponerDato(d);
          nuevo->ponerEnlace(lista);
          lista=nuevo;
        }        

        void printList(){
          Nodo<T> * temp =lista;

          while(temp!=0){
            cout<<temp->obtenerDato()<<endl;
            temp=temp->obtenerEnlace();               
          }
        }
    };

    #endif

Node.h

#ifndef _NODO_H 
#define _NODO_H


template <class T> class Nodo{

    protected:

    T dato;
    Nodo<T> * enlace;

    public:


    Nodo(T d){

        enlace=0;
        dato=d;

    }    


    Nodo(T d,Nodo * n){

        enlace=n;
        dato=d;

    }    


    Nodo<T>* obtenerEnlace() const{

        return enlace;     

    }


    void ponerEnlace(Nodo * n){

        enlace=n;   

    }

    void ponerDato(T d){

        dato=d;   

    }


    T obtenerDato() const{

        return dato;   

    }


};


#endif

c++

asked by Juan 12.01.2017 в 05:37

source

2 answers

How to see image in full screen when clicking on it? Create an own User Model and authenticate the entry

score 1 · Answer 1

void deleteAt(int p){
  Nodo<T>* temp = lista;

  if(p==1){
    lista=lista->obtenerEnlace();
    free(temp);
  }
  else{
    for(int i =0;i<p-2;i++) temp=temp->obtenerEnlace();

    Nodo<T>* temp2 =temp->obtenerEnlace();
    temp->ponerEnlace(temp2->obtenerEnlace());
    free(temp2);
  }
}

Suppose there are no items on the list ... What will happen if I call this function? It will try to access non-reserved memory and that's not cool ... even less if you also try to free it.

void deleteAt(int p){
  if( lista == 0 ) return;
  // ...
}

In C ++ the indexes start at 0. Let your function treat the first element of the list with the index 1 is contrary to the common norm and is prone to give errors. The correct thing would be to put:

if(p==0){
  lista=lista->obtenerEnlace();
  free(temp);
}

Forcing the first index to be 0 forces you to do rare algorithms:

for(int i =0;i<p-2;i++) temp=temp->obtenerEnlace();

Would not it be easier to do this?

while( p-- ) temp = temp->obtenerEnlace();

Although already made to prevent us from leaving the list:

while( p-- && temp ) temp = temp->obtenerEnlace();

If we analyze other possible problems we can look at temp2

Nodo<T>* temp2 = temp->obtenerEnlace();

What happens if temp or temp2 points to 0? Easy, the application will re-crack.

while( p-- && temp ) temp = temp->obtenerEnlace();

if( temp )
{
  Nodo<T>* temp2 = temp->obtenerEnlace();
  if( temp2 )
    temp->ponerEnlace(temp2->obtenerEnlace());

  free(temp2);
}

I know there are many checks, but it is the price to pay for working with dynamic memory.

I do not guarantee that this will solve your problem, since you have not provided a minimum compilable example, but rest assured that your code is quite insecure.

This confused me because at the beginning the question was labeled C ...

void insertValue(T d){
  Nodo<T> * nuevo =(Nodo<T>*)malloc(sizeof(Nodo<T>));
  nuevo->ponerDato(d);
  nuevo->ponerEnlace(lista);
  lista=nuevo;
}

In C ++ objects must be created with new and deleted with delete . malloc and free serve to reserve or free memory ... But they will not call the constructor or the destructor. The correct thing would be to do:

void insertValue(T d){
  Nodo<T> * nuevo = New Nodo<T>(d);
  nuevo->ponerEnlace(lista);
  lista=nuevo;
}

And replace this:

free(temp2);

Because of this:

delete temp2;

score 1 · Answer 2

There is no error in your code that leads to that behavior, despite being too little or not at all robust.

The error occurs when mixing input / output methods, and specifically, mix std::cin and std::cout with printf and scanf .

And where have you put a printf / puts / etc? Precisely in the call to system (implicitly).

If you mix methods, you should effectively clean or empty the buffers (actually, synchronize). Also keep in mind that in some cases the buffers may have been implicitly synchronized. For example, when the object std::endl is used, in an instruction of type std::cout << std::endl; , the buffer of the object std::cout will be synchronized automatically, that is, internally, a call will be made to std::cout.flush() , so it does not you would need to add that statement explicitly before changing to printf , for example.

Finally, and although it is not part of the question, I will answer several questions that have arisen when you see your code.

Although in C, C ++ and other programming languages the indexes are controlled from 0 to n-1 in an indexed data type, as eferion says, it does not mean that you can not create a class whose indexes are 1 a n. That's totally valid , although I would not advise it.

You're mixing C with C ++ , and that can lead to serious errors. For example, you are creating objects with malloc , and (as eferion comments as well), this causes the class constructor not to be called. In C ++ objects are created with new and are destroyed with delete , which calls the destructor defined in the class (or the default destructor).

You do not use the builders of your classes to reserve dynamic memory (related to the above), but the worst is that you do not release it by destructors (you have put a free that is insufficient). The result is that after creating a list, the program will inevitably have memory leaks or memory leaks . I recommend you use the valgrind tool to check this.

Always create destructors for all the classes that you define, because if the instances with new , you must release their objects with delete . If you declare static objects (without pointers), as in the Lista<std::string> lista; statement, calls to destructors of classes will be implicitly , that is, even if you do not see the call in your code, this will be done at the end of the object's life cycle.

Although it is correct to handle pointers using literals such as 0 (the null pointer), there are conventions of names. Use the constant NULL of the header cstdlib for it, or the reserved word nullptr (C ++ 11), since both refer to the null pointer.

Review the names of the libraries you use. Specifically, in C ++ there is a standard library for each of C's, but whose name varies slightly (a c is added at the beginning and the .h is removed). The library string.h of C would correspond to the library cstring of C ++.

Since you manage strings through the class std::string , and do not do anything complex with them, you do not need the library cstring , which handles the strings of type char * . Instead, the library that handles C ++ strings ( std::string ) is the library string (does not have the c , which indicates that it does not have to do with C, it is only C ++).

Finally, I recommend you check your code, it is very unsafe and can lead to errors at runtime. For example, in the function deleteAt(int p) , you should first check that the index is within the limits you have defined (from 1 to n, or from 0 to n - 1). Therefore, check your knowledge about Exceptions in C ++.

I hope this clarifies your doubts and helps you improve the quality of your code (I say it without acrimony).