0

Estoy haciendo un programa con un menu para cargar , eliminar y visualizar una lista enlazada atraves de la clase lista y nodo. El problema es que a veces se cargan todos los valores normalmente , y otras el programa crashea depues de cargar uno o dos valores.Si intento crear una lista y la cargo fuera del do while del menú, el programa solo carga 17 elementos. Si intento cargar uno mas, el programa se cierra.

#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;
}

Lista.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

Nodo.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
2
  • intenta agregando el break en el case 3:...
    – MindLerp
    Commented el 12 ene. 2017 a las 5:31
  • Falta la implementación de Nodo. Lo que indicas como Nodo.h es realmente Lista.h
    – eferion
    Commented el 12 ene. 2017 a las 10:52

2 respuestas 2

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);
  }
}

Supongamos que no hay elementos en la lista... ¿Qué sucederá si llamo a esta función? Intentará acceder a memoria no reservada y eso no mola... menos aún si además intenta liberarla.

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

En C++ los índices empiezan en 0. Que tu función trate el primer elemento de la lista con el índice 1 es contrario a la norma común y es propenso a dar errores. Lo correcto sería poner:

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

El obligar a que el primer índice sea 0 te obliga a hacer algorítmica rara:

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

¿No sería más sencillo hacer esto?

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

Aunque ya puestos a prevenir que nos salgamos de la lista:

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

Si analizamos otros posibles problemas podemos fijarnos en temp2

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

¿Qué sucede si temp o temp2 apunta a 0? Fácil, la aplicación volverá a cascar.

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

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

  free(temp2);
}

Se que son muchas comprobaciones, pero es el precio a pagar por trabajar con memoria dinámica.

No garantizo que esto solucione tu problema, ya que no has proporcionado un ejemplo mínimo compilable, pero ten por seguro que tu código es bastante inseguro.


Esto me despistó porque al principio estaba etiquetada la pregunta como C...

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

En C++ los objetos hay que crearlos con new y borrarlos con delete. malloc y free sirven para reservar o liberar memoria... Pero no llamarán al constructor ni al destructor. Lo correcto sería hacer:

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

Y sustituir esto:

free(temp2);

Por esto:

delete temp2;
6
  • Gracias por las correciones!! el problema parece estar al insertar nodos...
    – user22178
    Commented el 12 ene. 2017 a las 18:35
  • Cree una lista por fuera del menú y llega a cargar 17 elemntos en la lista...luego el programa se cierra :(..
    – user22178
    Commented el 12 ene. 2017 a las 18:55
  • @Juan ¿Podrías confirmarme si la edición de mi respuesta soluciona tu problema?
    – eferion
    Commented el 16 ene. 2017 a las 16:45
  • No... el problema se solcionó al limpiar el buffer luego de cada cin.
    – user22178
    Commented el 17 ene. 2017 a las 18:31
  • Eso te está pasando entonces por mezclar cin y cout con system
    – eferion
    Commented el 17 ene. 2017 a las 18:38
1

No existe ningún error en tu código que lleve a ese comportamiento, a pesar de ser muy poco o nada robusto.

El error se da al mezclar métodos de entrada/salida, y en concreto, mezclar std::cin y std::cout con printf y scanf.

¿Y dónde has puesto un printf/puts/etc? Precisamente en la llamada a system (de manera implícita).

Si mezclas métodos, efectivamente debes limpiar o vaciar los buffers (en realidad, sincronizar). Ten en cuenta, además, que en algunas ocasiones los buffers pueden haberse sincronizado implícitamente. Por ejemplo, cuando se utiliza el objeto std::endl, en una instrucción del tipo std::cout << std::endl;, el buffer del objeto std::cout se sincronizará automáticamente, es decir, internamente, se hará una llamada a std::cout.flush(), por lo que no necesitarías añadir esa sentencia explícitamente antes de cambiar a printf, por ejemplo.

Finalmente, y aunque no sea parte de la pregunta, te responderé a varias cuestiones que han surgido al ver tu código.

  1. Aunque en C, C++ y otros lenguajes de programación los índices se controlen de 0 a n-1 en un tipo de datos indizado, como comenta eferion, no significa que no puedas crear una clase cuyos índices vayan de 1 a n. Eso es totalmente válido, aunque yo no te lo aconsejaría.

  2. Estás mezclando C con C++, y eso puede llevar a errores graves. Por ejemplo, estás creando objetos con malloc, y (tal y como comenta eferion también), esto produce que no se llame al constructor de la clase. En C++ los objetos se crean con new y se destruyen con delete, que llama al destructor definido en la clase (o al destructor por defecto).

  3. No utilizas los constructores de tus clases para reservar memoria dinámica (relacionado con lo anterior), pero lo peor es que no la liberas mediante destructores (has puesto un free que resulta insuficiente). El resultado es que tras la creación de una lista, el programa inevitablemente tendrá memory leaks o fugas de memoria. Te recomiendo utilizar la herramienta valgrind para comprobar esto.

  4. Crea siempre destructores para todas las clases que definas, pues si las instancias con new, deberás liberar sus objetos con delete. Si declaras objetos estáticos (sin punteros), como en la sentencia Lista<std::string> lista;, las llamadas a los destructores de las clases se harán implícitamente, es decir, aunque no veas la llamada en tu código, esta se realizará al final del ciclo de vida del objeto.

  5. Aunque es correcto manejar los punteros utilizando literales como 0 (el puntero nulo), existen convenciones de nombres. Utiliza la constante NULL de la cabecera cstdlib para ello, o bien la palabra reservada nullptr (C++ 11), ya que ambas hacen referencia al puntero nulo.

  6. Revisa los nombres de las bibliotecas que utilices. En concreto, en C++ existe una biblioteca estándar para cada una de las de C, pero cuyo nombre varía ligermanete (se añade una c al principio y se elimina el .h). La biblioteca string.h de C se correspondería con la biblioteca cstring de C++.

  7. Como manejas cadenas a través de la clase std::string, y no haces nada complejo con ellas, no te hace falta la biblioteca cstring, que maneja las cadenas del tipo char *. En su lugar, la biblioteca que maneja cadenas de C++ (std::string) es la biblioteca string (no lleva la c, lo que indica que no tiene que ver con C, es sólo de C++).

  8. Finalmente, te recomiendo revisar tu código, pues es muy inseguro y puede llevar a errores en tiempo de ejecución. Por ejemplo, en la función deleteAt(int p), deberías en primer lugar comprobar que el índice está dentro de los límites que has definido (de 1 a n, o bien de 0 a n - 1). Por tanto, revisa tus conocimientos sobre excepciones en C++.

Espero que esto aclare tus dudas y te sirva de ayuda para mejorar la calidad de tu código (lo digo sin acritud).

2
  • No estoy de acuerdo con lo de que haya que crear destructores siempre ya que el compilador sabe crear un destructor por defecto que sirve en muchísimas ocasiones (aplicable también para el constructor por defecto y el constructor copia). Solo es necesario implementar uno propio cuando se gestiona memoria dinámica de forma manual o cuando el destructor debe hacer operaciones adicionales no contempladas (por ejemplo llamar a un log, avisar a otro proceso, cosas así...)
    – eferion
    Commented el 6 mar. 2018 a las 14:55
  • @eferion tienes toda la razón sobre los constructores y destructores por defecto. Sin embargo, creo que es una buena práctica y que no hace daño crear un destructor vacío explícitamente, sobre todo al principio. Por supuesto, cuando tienes experiencia y sabes lo que haces, no hace falta para nada.
    – Eequiis
    Commented el 6 mar. 2018 a las 15:05

Tu Respuesta

By clicking “Publica tu respuesta”, you agree to our terms of service and acknowledge you have read our privacy policy.