Klasa lista, problem z wyczyszczeniem listy.

Stowrzyłem klasę lista, która chce teraz wyczyścić(metoda wyczysc na samym dole klasy), problem w tym, że jeśli dodaje: "delete korzen" to program sie wysypuje, a bez niego nie usuwa mi ostatniego elementu z listy.
Jak mogę to naprawić?

#include <string>
#include <iostream>

using namespace std;

class Lista
{
	struct Element
	{
		int ilosc;
		string nazwa;
		Element *nastepny;
	};
	Element *korzen, *ostatni;
public: 
	Lista()
	{
		korzen = ostatni = NULL;
	}
	Lista(const Lista &list)
	{
		if (list.korzen == list.ostatni)
			korzen = ostatni = NULL;
		else
		{
			Element *pom = list.korzen;
			korzen = new Element;
			korzen->ilosc = pom->ilosc;
			korzen->nazwa = pom->nazwa;
			ostatni = korzen;
			while(pom!=list.ostatni)
			{
				pom = pom->nastepny;
				ostatni->nastepny = new Element;
				ostatni = ostatni->nastepny;
				ostatni->ilosc = pom->ilosc;
				ostatni->nazwa = pom->nazwa;
			}

		}
	}
	~Lista()
	{
		Element *pom=korzen;
		while(korzen!=ostatni)
		{
			pom = korzen;
			korzen = korzen->nastepny;
			delete pom;
		}
		delete korzen;
	}

	void dodaj(int ilosc, string nazwa)
	{
		if (ostatni!=NULL)
		{
			ostatni->nastepny = new Element;
			ostatni = ostatni->nastepny;
		}
		else
		{
			korzen = ostatni = new Element;
		}
		ostatni->ilosc = ilosc;
		ostatni->nazwa = nazwa;
	}
	void wyswietl()
	{
		Element *pom=korzen;
		
		while(pom!=ostatni)
		{
			cout << pom->nazwa << "   " << pom->ilosc<<endl;
			pom = pom->nastepny;
		}
		cout << ostatni->nazwa << "  " << ostatni->ilosc<<endl;
	}
	void usun()
	{
		Element *pom = korzen;
		while(pom!=korzen)
		{
			if (pom);
		}
	}
	void wyczysc()
	{
		Element *pom = korzen;
		while (korzen != ostatni)
		{
			pom = korzen;
			korzen = korzen->nastepny;
			delete pom;
		}
		//delete korzen;
	}

};


int main()
{
	Lista list;
	list.dodaj(5, "bulki");
	list.dodaj(5, "mleko");
	list.dodaj(5, "mleko");
	list.wyswietl();
	Lista l1 = list;
	l1.wyswietl();
	l1.wyczysc();
	l1.wyswietl();

    return 0;
}
 

Wysypie się juz z samego faktu, że po wyczysc nigdzie nie ustawiasz korzenia ani ostatniego na nulle, wiec jak tylko uruchomi się destruktor to oba zostaną po raz drugi "zwolnione".

Przecież w destruktorze masz identyczny kod co w wyczysc() (razem z delete korzen), co wprost domaga się, by destruktor był implementowany jako

~Lista()
{
   wyczysc();
}

Teraz jeszcze wyraźniej widać, że jeżeli ręcznie wołasz wyczysc(), to przy zniszczeniu obiektu to zostanie wołane drugi raz. Więc nie wołaj ręcznie wyczysc() (daj tę funkcję do private) i/lub po wołaniu delete ustaw korzen i ostatni na nullptr.

BTW funkcja usun() wygląda co najmniej bezsensownie.

Rzeczywiście, udało mi sie naprawić, ale teraz mam inny problem.

Mianowicie, chce usuwać z listy te rzeczy, ktore sa w metodzie podane jako argument. Jak mogę przenieść by wskaznik pokazujący na usuwany pokazywał na nastepny, po usuwanym?

 

#include <string>
#include <iostream>

using namespace std;

class Lista
{
	struct Element
	{
		int ilosc;
		string nazwa;
		Element *nastepny;
	};
	Element *korzen, *ostatni;
public: 
	Lista()
	{
		korzen = ostatni = NULL;
	}
	Lista(const Lista &list)
	{
		if (list.korzen == list.ostatni)
			korzen = ostatni = NULL;
		else
		{
			Element *pom = list.korzen;
			korzen = new Element;
			korzen->ilosc = pom->ilosc;
			korzen->nazwa = pom->nazwa;
			ostatni = korzen;
			while(pom!=list.ostatni)
			{
				pom = pom->nastepny;
				ostatni->nastepny = new Element;
				ostatni = ostatni->nastepny;
				ostatni->ilosc = pom->ilosc;
				ostatni->nazwa = pom->nazwa;
			}

		}
	}
	~Lista()
	{
		Element *pom=korzen;
		while(korzen!=ostatni)
		{
			pom = korzen;
			korzen = korzen->nastepny;
			delete pom;
		}
		delete korzen;
	}

	void dodaj(int ilosc, string nazwa)
	{
		if (ostatni!=NULL)
		{
			ostatni->nastepny = new Element;
			ostatni = ostatni->nastepny;
		}
		else
		{
			korzen = ostatni = new Element;
		}
		ostatni->ilosc = ilosc;
		ostatni->nazwa = nazwa;
	}
	void wyswietl()
	{
		Element *pom=korzen;
		if (korzen != ostatni)
		{

			while (pom != ostatni)
			{
				cout << pom->nazwa << "   " << pom->ilosc << endl;
				pom = pom->nastepny;
			}
			cout << ostatni->nazwa << "  " << ostatni->ilosc << endl;
		}
	}
	void usun(string napis)
	{
		Element *pom = korzen;
		Element *poprzedni;
		while(pom!=ostatni)
		{
			
			if (pom->nazwa == napis)
			{
				//tutaj chce przesunac wskaznik z poprzedniego na nastepny
				//czyli "zalatac dziure" po usunietym elemencie
				poprzedni->nastepny = pom->nastepny;
				delete pom;
				break;
			}
			poprzedni = pom;
			pom = pom->nastepny;
		}
	}
	void wyczysc()
	{
		Element *pom = korzen;
		while (korzen != ostatni)
		{
			pom = korzen;
			korzen = korzen->nastepny;
			delete pom;
		}
		korzen = ostatni = NULL;
	}
};


int main()
{
	Lista list;
	list.dodaj(5, "bulki");
	list.dodaj(5, "mleko");
	list.dodaj(5, "mleko");
	Lista l1 = list;
	l1.usun("bulki");
	l1.wyswietl();

    return 0;
}

To jakby się prosiło o rozbicie na warunki czy chcesz usunąć korzeń czy coś co jest dalej (tj musisz sprawdzić czy usuwasz korzeń czy nie). Zmiana korzenia w funkcji usun() to chyba niezbyt dobre rozwiązanie, funkcja wyswietl() nie będzie przez to działać poprawnie. Wypadałoby wprowadzić nową zmienną i iterować od następnego po korzeniu a pierwszą zmienną od korzenia.

Coś tam Ci dzwoni, ale nie wiesz w którym kościele ;) Najpierw zaimplementuj poprawnie metodę dodaj i konstruktor kopiujący. dodaj nie traktuje poprawnie korzenia, konstruktor kopiujący, jak już poprawnie zaimplementujesz inicjalizację korzenia, nie zadziała poprawnie dla listy jednoelementowej.

several napisał(a):

Coś tam Ci dzwoni, ale nie wiesz w którym kościele ;) Najpierw zaimplementuj poprawnie metodę dodaj i konstruktor kopiujący. dodaj nie traktuje poprawnie korzenia, konstruktor kopiujący, jak już poprawnie zaimplementujesz inicjalizację korzenia, nie zadziała poprawnie dla listy jednoelementowej.

Hmmm co masz na myśli, że konsturktor kopiujący nie zadziała?

Poprawiłem troszke i zrobiłem z tego listę dwukierunkową, czy są tu jakieś bugi których nie widzę?

#include <iostream>
using namespace std;


class lista
{
	struct Element
	{

		int wartosc;
		Element *poprzedni, *nastepny;
	};

	Element *pierwszy, *ostatni;
public:	
	lista()
	{
		pierwszy = ostatni = NULL;
	}
	~lista()
	{
		Element *pom;
		while(pierwszy!=ostatni)
		{
			pom = pierwszy;
			pierwszy = pierwszy->nastepny;
			delete pom;
		}
		delete pierwszy;
	}
	lista(const lista &kopia)
	{
		if (kopia.pierwszy == kopia.ostatni)
		{
			pierwszy = ostatni = NULL;
		}
		else
		{
			Element* pom = kopia.pierwszy;
			pierwszy = new Element;
			pierwszy->wartosc = pom->wartosc;
			ostatni = pierwszy;
			while (pom != kopia.ostatni)
			{
				pom = pom->nastepny;
				ostatni->nastepny = new Element;
				ostatni->nastepny->poprzedni = ostatni;
				ostatni = ostatni->nastepny;
				ostatni->wartosc = pom->wartosc;
			}
		}
	}
	void dodaj_przod(int n)
	{
		if (pierwszy == ostatni)  //jesli lista jest pusta, lub ma tylko 1 element
		{
			if (pierwszy == NULL)
			{
				pierwszy = ostatni = new Element;
				pierwszy->wartosc = n;
			}
			else
			{
				Element *pom = new Element;
				pom->wartosc = n;
				pom->poprzedni = NULL;
				pom->nastepny = ostatni;
				pierwszy = pom;
			}

		}
		else ////jesli lista ma przynajmniej 2 elementy
		{
			Element *pom = new Element;
			pom->wartosc = n;
			pom->poprzedni = NULL;
			pom->nastepny = pierwszy;
			pierwszy = pom;
		}
	}
	void dodaj_tyl(int n)
	{
		if (pierwszy == ostatni) //jesli lista jest pusta, lub ma tylko 1 element
		{
			if (pierwszy == NULL)
			{
				pierwszy = ostatni = new Element;
				pierwszy->wartosc = n;
			}
			else
			{
				Element *pom = new Element;
				pom->wartosc = n;
				pom->poprzedni = pierwszy;
				pom->nastepny = NULL;
				ostatni = pom;
			}
		}
		else //jesli lista ma przynajmniej 2 elementy
		{
			ostatni->nastepny = new Element;
			ostatni->nastepny->poprzedni = ostatni;
			ostatni = ostatni->nastepny;
			ostatni->wartosc = n;
		}
	}
	void usun_tyl()
	{
		if (pierwszy == ostatni)
		{
			pierwszy = ostatni = NULL;
		}
		else
		{
			ostatni = ostatni->poprzedni;
			delete ostatni->nastepny;
			ostatni->nastepny = NULL;
		}
	}
	void usun_przod()
	{
		if(pierwszy==ostatni)
		{
			pierwszy = ostatni = NULL;
		}
		else
		{
			pierwszy = pierwszy->nastepny;
			delete pierwszy->poprzedni;
			pierwszy->poprzedni == NULL;
		}
	}
	void wypisz()
	{
		if (pierwszy != ostatni)
		{
			Element *pom = pierwszy;
			while (pom != ostatni)
			{
				cout << pom->wartosc;
				pom = pom->nastepny;
			}
			if(ostatni!=NULL)
			cout << ostatni->wartosc << endl;
		}

	}
};


int main()
{
	lista l;
	l.dodaj_przod(3);
	l.usun_tyl();
	l.wypisz();
	lista l1 = l;
	l1.dodaj_przod(3);
	l1.usun_przod();
	l1.wypisz();

	

    return 0;
} 

mistrzuniu1 napisał(a):

Hmmm co masz na myśli, że konsturktor kopiujący nie zadziała?

Ten if

if (kopia.pierwszy == kopia.ostatni)
{
    pierwszy = ostatni = NULL;
}

w przypadku listy jednoelementowej, kiedy pierwszy i ostatni element mają tą samą wartość sprawi, że do nowej listy wpiszesz nulle zamiast poprawnych wartości.