Problem z wstawianiem do drzewa, c

Mam napisać program, który będzie zliczał wystąpienia słów i umieszczał te słowa w drzewie alfabetycznym/ Od 3 dni męczę się z następującym kodem:

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

struct Node
{
	int licznik;
	char* slowo;
	struct Node* lewy;
	struct Node* prawy;
};
typedef struct Node* Tree;
void konwersjanaslowo(char* slowo)
{
	for(int i=0; slowo[i]!=0; i++)
	{
		if(slowo[i]>='a' && slowo[i]<='z'){slowo[i]=slowo[i]+'A'-'a';}
		else
		{
			if(slowo[i]<'A' || slowo[i]>'Z'){slowo[i]=0; return;}
		}
	}
}
void kopiuj(char* slowo1, char* slowo2)
{
	int i=0;
	while(slowo1[i]!=0)
	{
		slowo2[i]=slowo1[i];
		i++;
	}
	slowo2[i]=0;
}
short int porownaj(char* slowo1, char* slowo2)
{
	int i=0;
	while(slowo1[i]!=0 && slowo2[i]!=0)
	{
		if(slowo1[i]<slowo2[i]){return -1;}
		else{if(slowo1[i]>slowo2[i]){return 1;}}
		i++;
	}
	if(slowo1[i]==slowo2[i]){return 0;}
	else
	{
		if(slowo1[i]==0){return -1;}
		else{return 1;}
	}
}
void swap(Tree ptr1, Tree ptr2)
{
	Tree temp=ptr1;
	ptr1=ptr2;
	ptr2=temp;
}
void wstaw(char* slowo, Tree* ptr, Tree* najczestsze, int top)
{
	if(*ptr==NULL)
	{
		*ptr=malloc(sizeof(struct Node));//!!!!!!!
		(*ptr)->licznik=1;
		(*ptr)->lewy=NULL;
		(*ptr)->prawy=NULL;
		kopiuj(slowo, (*ptr)->slowo);
		if(najczestsze[top]==NULL)
		{
			int i=0;
			while(najczestsze[i]!=NULL){i++;}
			najczestsze[i]=*ptr;
		}
	}
	else
	{
		if(porownaj(slowo, (*ptr)->slowo)==0)
		{
			(*ptr)->licznik++;
			short int znaleziono=0, i=0;
			while(i<top && znaleziono==0)
			{
				if(najczestsze[i]==(*ptr)){znaleziono=1;}
				else{i++;}
			}
			if(znaleziono==1)
			{
				int k=i;
				while(najczestsze[k]->licznik < (*ptr)->licznik){k--;}
				k++;
				if(k!=i){swap(najczestsze[k], najczestsze[i]);}
			}
			else
			{
				if(najczestsze[top]->licznik < (*ptr)->licznik)
				{
					while(najczestsze[top-1]->licznik < (*ptr)->licznik){top--;}
					najczestsze[top]=(*ptr);
				}
			}
		}
		else
		{
			if(porownaj(slowo, (*ptr)->slowo)==-1){wstaw(slowo, &((*ptr)->lewy), najczestsze, top);}
			else{wstaw(slowo, &((*ptr)->prawy), najczestsze, top);}
		}
	}
}
void zwolnij_drzewo(Tree* ptr)
{
	if(*ptr!=NULL)
	{
		zwolnij_drzewo(&((*ptr)->lewy));
		zwolnij_drzewo(&((*ptr)->prawy));
		free(*ptr);
		*ptr=NULL;
	}
}
void wypisz(Tree* tab, int rozmiar)
{
	for(int i=0; i<rozmiar; i++)
	{
		printf("%s %d", tab[i]->slowo, tab[i]->licznik);
	}
}
int main(int argc, char *argv[])
{
	int top=0;
	if(argc>1)
	{
		for(int i=0; argv[1][i]!=0; i++)
		{
			top=top*10+(int)argv[1][i]-48;
		}
	}else{top=100;}
	Tree* najczestsze=malloc(top*sizeof(Tree));
	for(int i=0; i<top; i++){najczestsze[i]=NULL;}
	Tree ptr=NULL;
	char slowo[50];
	while(scanf("%49s", slowo)!=EOF)
	{
		konwersjanaslowo(slowo);
		wstaw(slowo, &ptr, najczestsze, top);
	}
	wypisz(najczestsze, top);
	zwolnij_drzewo(&ptr);
	free(najczestsze);
	return 0;
}

Wszystko ładnie się kompiluje, ale jak próbuję to zaprząc do pracy nad plikiem:

To program mi się wysypuje. Wydaje mi się, że chodzi o zaznaczoną wykrzyknikami linijkę. Skąd te przypuszczenia? Wpisywałem instrukcję printf(“Jestem tu\n”) w różnych miejscach i kiedy wpisałem przed tą linijką, to napis wyświetlał się 2 razy. Kiedy wstawiłem za tą linijką, to napis wyświetlił się tylko raz. Stąd mój wniosek był taki, że problem powstaje przy wstawianiu drugiego słowa do drzewa. Czy ktoś może wie, dlaczego tak się dzieje?

P.S. Można pominąć część, w której program pracuje nad tablicą najczestsze, bo po okomentowaniu tego fragmentu kodu problem jest ten sam.

  1. Czemu nie używasz strcmp() i strcpy() ?

  2. Czemu funkcja wstaw odpowiada za 2 różne rzeczy - tworzenie drzewa i wstawianie elementów do niego (zaciemnianie kodu), 1 funkcja odpowiada 1 zadaniu które definiuje już jej nazwa.

  3. Mogę nie pamiętać wskaźników, ale czy to:

    *ptr=malloc(sizeof(struct Node));

jest poprawne?

Wydaje mi się że w taki sposób to ustawiasz wartość, ale wskaźnik wskazuje cały czas na adres NULL i to co tworzysz, adres zarezerwowanej pamięci dla tego ląduje w wartości która w pamięci zaczyna się od NULL, więc zapisujesz obszar pamięci niezarezerwowany przez program.

Więc powinno być:

ptr = (struct Node *) malloc(sizeof(struct Node));
  1. Po co licznik w drzewie?

  2. Wydaje mi się że błędów jest znacznie więcej, nie jestem ich tylko pewny bo nie programuje w C a w C++ programowałem dawno temu.

EDIT:

  1. Czemu masz tak pokracznego typedefa? Nie lepiej tak:

    typedef struct Node Tree;

Sam się gubisz w swoim kodzie:

void swap(Tree ptr1, Tree ptr2)
{
}
void wstaw(char* slowo, Tree* ptr, Tree* najczestsze, int top)
{
}

Ja to widzę tak, naklepałeś kodu jak dziki który nie działa bo jest niepoprawny. Proponowałbym Ci zacząć od nowa, napisz jedną funkcję i ją przetestuj czy działa. Zacząłbym od funkcji która tworzy nowe drzewo, następnie która je usuwa (już ją masz).

EDIT 2:

Albo nazwy funkcji i zmiennych piszesz po polsku albo po angielsku, zdecyduj się.

Nie wiem czy przypadkiem przy wstawianiu elementów drzewa nie trzeba go przesortować.

@Fizyda

  1. Programuję na dobre dopiero od pół roku i nie wyrobiłem sobie nawyku szukania gotowych funkcji. Jak umiem coś robić, to po prostu samemu piszę funkcję.

  2. Funkcja tworząca drzewo byłaby tym samym, co zawartość pierwszego ifa funkcji wstaw, więc chyba nie ma sensu robić 2 różnych funkcji, skoro i tak w funkcji wstaw ten if musi się znaleźć.

  3. Może być i tak. Jak patrzyłem na kod znajomego, to on miał z rzutowaniem.

  4. Licznik zlicza, ile razy wystąpiło dane słowo(program ma wypisać najczęściej występujące słowa).

  5. Tak nam pokazywał wykładowca na uczelni, więc sądziłem, że jest ok. Przy twoim typedefie funkcja wstaw będzie musiała przyjmować wartość typu Tree**. A tak na prawdę, to samego struct Node używam tylko w mallocu.

Dzięki za radę :slight_smile:

A z tymi nazwami, to część po prostu nazywam tak, jak nazywaliśmy zmienne na ćwiczeniach, a część pierwszym, co mi wpadnie do głowy i wiąże się z działaniem funkcji.

@nintyfan:

Funkcja wstawiająca działa tak, że drzewo, po wstawieniu, nadal jest uporządkowane.

Edit. Dzięki za wskazówki, znalazłem już błąd. Nie zaalakowałem dynamicznie słowa, wystarczyło przed strcpy((*ptr)->slowo, slowo) wstawić linijkę:

(*ptr)->slowo=calloc(strlen(slowo)+1, sizeof(char));

I to jest błąd bo tracisz tylko czas i siły na tworzenie czegoś co już jest zrobione. Naukę programowania powinno się zacząć od poznania składni, a następnie od poznania środowiska. Chociażby przejrzenia dokumentacji i zobaczenia jakie funkcje i czym zajmujące się znajdują się w środowisku. W przypadku C/C++ jest tego naprawdę niewiele w porównaniu do innych języków.

Wszędzie robisz funkcje zwracające void, a może zrób zwracające bool i w przypadku wstawiania elementu do drzewa poinformuj czy udało się wstawić element, skąd masz wiedzieć czy dodałeś go cze nie. W tedy sprawdzasz tylko czy drzewo istnieje jeśli nie to zwracasz false, a jak tak to dodajesz.

Dlaczego niby musiałbyś tak zrobić? Pytam bo chcę się dowiedzieć czy ja już źle pamiętam przez pyły czasu wskaźniki czy coś Ty wymyśliłeś.

Problem z boolem jest taki, że w C nie ma takiego typu :wink:

Samo drzewo musi być wskaźnikiem na strukturę, gdyż jest tworzone przy pomocy malloc, a żeby odbierać to, co jest tworzone przez malloc, potrzebne są wskaźniki. A do funkcji muszę przesyłać adres tego drzewa, abym mógł coś w nim zmieniać(inaczej miałbym normalne przekazywanie przez wartość).

I jeszcze zadam jedno pytanie, bo nie jestem pewien, czy poprawnie usuwam drzewo. Mam funkcję:

void usun_drzewo(Tree* ptr)
{
    if(*ptr!=NULL)
    {
        usun_drzewo(&((*ptr)->lewy));
        usun_drzewo(&((*ptr)->prawy));
        free((*ptr)->slowo);
        free(&((*ptr)->licznik));//zamiast tej linijki chciałem dać po prostu free(*ptr), ale wtedy miałem błąd przy uruchamianiu programu
        //jak tutaj dopisałem free(*ptr), to też był błąd działania
        *ptr=NULL;
    }
}

I zastanawiam się, czy ona w poprawny sposób usuwa drzewo.

Zawsze możesz użyć inta.

Nie zgodzę się z twoim uzasadnieniem. Szczerze to dziwie się że Twój kod działa poprawnie.

Nie do końca poprawnie usuwasz drzewo.

  1. nie musisz zwalniać pamięci licznika bo jej nie rezerwujesz dynamicznie. Funkcji free używasz do zwolnienia pamięci zarezerwowanej dynamicznie przy pomocy malloc, której to adres przechowujesz we wskaźnikach.

  2. w wywołaniach rekurencyjnych zrezygnowałbym z &, ale tego nie jestem pewny

PS. Co do twojego uzasadnienia czemu robisz tak typedefa, nie zmuszaj mnie bym ściągnął kompilator z IDE do C/C++.

  1. A jak wywalę linijkę z usuwaniem licznika to będzie ok? Nie będzie wycieku pamięci? Bo jak zamiast tej linijki wpisuję free(*ptr), to wywala mi błąd.

2)W wywołaniach rekurencyjnych musi być &, bo funkcja przyjmuje wartość typu Tree*, a (*ptr)->lewy jest typu Tree.

  1. wycieku pamięci nie będzie, ale i tak musisz zrobić free na ptr bo rezerwujesz pamięć gdy tworzysz obiekt. Tylko nie free(*ptr) tylko free(ptr). Zwalniasz zawartość wskaźnika czy miejsce na które wskazuje?

PS. Nie rozumiesz wskaźników.

  1. A czy bez & masz błąd? To co robisz to przekazujesz adres wskaźnika i kasujesz wskaźnik traktując go jak wskaźnik typu Tree, dlatego mówię Ci że masz źle typdefa. Sam gubisz się w kodzie i nie wiesz co się w nim dzieje, coś niby kasujesz ale sam nie wiesz co. Coś niby rezerwujesz, ale właściwie co.

To że kompilator się nie buntuje nie znaczy że to ma sens. Gdyby kompilator wiedział czy kod jest poprawny logicznie to nie byłbyś potrzebny bo kompilator sam stworzyłby kod.

Dobra, już wiem co się stało. W kodzie miałem jakąś literówkę. Teraz wygląda to tak:

void usun_drzewo(Tree* ptr)
{
	if(*ptr!=NULL)
	{
		usun_drzewo(&((*ptr)->lewy));
		usun_drzewo(&((*ptr)->prawy));
		free((*ptr)->slowo);
		free(*ptr);
		*ptr=NULL;
	}
}

i wszystko pięknie się wykonuje. Co do sensowności typedefa- kłócił się nie będę, tak kazał robić wykładowca na uczelni, więc jest to prawdopodobnie sensowne.

Z ty to bym się kłócił akurat i to ostro.

Twój typedef ma sens, ale w konkretnych sytuacjach tutaj tego nie widzę. Zwłaszcza że się dopiero uczysz.

Nie wiem jak dokładnie się pisze w języku  C++, aby dodać czynności  na  dany obiekt, ale chce cię poinformować że do każdego ogólnego słowa nie otwierasz i zamykasz go (). Poza tym tworzysz dwa elementy a nie jeden dany.

 

void kopiuj(char* slowo1, char* slowo2)
{
	int i=0;
	while(slowo1[i]!=0)
	{
		slowo2[i]=slowo1[i];
		i++;
	}
	slowo2[i]=0;

 

Odstęp też jest ważny n.p tu

*ptr=malloc(sizeof(struct Node));

Musisz oddzielać je i odpowiednio ustawiać w kolejności n.p

ptr = (struct Node *) malloc (sizeof(struct Node)) ;

Więcej ci nie powiem,patrząc się na górne komentarze,posłuchaj się lepiej użytkownika Fizyda, dobrze ci poradził !!!

Cóż - inną sprawą jest błędność tej konstrukcji, a inną odstępy.

Spacje to akurat zabieg czysto estetyczny mający poprawić czytelność kodu. Fakt są języki gdzie spacje są częścią składni języka np. F#, Scala i OCaml chyba też. W Pythonie z tego co mi wiadomo znaczenie mają tabulatory. W C/C++ czy napiszesz

int x=0,y=2;double z;

czy

int x=0, y=2;
double z;

albo

int x=0;
int y=2;
double z;

to tylko kwestia czytelności. Podobnie ze stylem nazewnictwa zmiennych i nazw funkcji. Trzymanie się jednego stylu poprawia czytelność, ale nie każdemu wszystko odpowiada.

Podobnie ma się sprawa z tym jak wygląda funkcja, możesz mieć

int funcOne(int a, double b)
{
    // function body
}

lub

int funcOne(int a, double b) {
    // function body
}

a nawet

int funcOne(int a, double b) {

    // function body
}

i wiele więcej. Kwestia tego co kto lubi. Ale nie zawsze warto przekładać swoje nawyki często dziwne bo nawet gdy rozwijamy kod sami to czasami zachodzi potrzeba pokazania go komuś innemu w celu np. weryfikacji poprawności jakiegoś rozwiązania/algorytmu i jeśli mamy sporo swoich dziwactw ktoś może mieć problem z odczytaniem takiego kodu.

Warto jednak pisać tak jak jest dla nas czytelnie. Osobiście styl nazewnictwa czy styl programowania powinno się dostosowywać do odpowiedniej sytuacji w kodzie oraz tego w jakim paradygmacie programowania piszemy. Np. nie ma se:nsu forsować stylu wielbłądziego w nazwach funkcji dla kodu pisanego strukturalnie (proceduralnego).

Np, gdy mamy taką sytuacje:

bool decision = flase;
int x = 0;

if(decision)
{
    x = 2;
}
else
{
    x = 1;
}

przykład jest poprawny i czytelny, ale żeby skrócić kod zakładając że w if’ie nie będziemy nic więcej robić warto napisać tak:

bool decision = flase;
int x = 0;

if(decision)
    x = 2;
else
    x = 1;

mamy przez to 4 linijki krótszy kod i czytelniejszy. Ale to nie jest najlepszy sposób bo jeszcze prościej można to zrobić, a zachowując czytelność

bool decision = flase;
int x = 0;

x = decision ? 2 : 1;

Takim oto sposobem z 11 lini zrobiliśmy 4. To się często przydaje zwłaszcza gdy stosuje się techniki/zasady dobrego programowania, np zasadę że funkcja musi mieścić się na ekranie, jeśli musimy scrlować powinniśmy rozbić ją na 2 mniejsze.

Stosowanie takich prostych zasad naprawdę podnosi czytelność pisanego kodu, a dodatków np jak ta wspomniana wyżej dba o to by funkcja zajmowała się jednym zadaniem a nie kilkoma co często się zdarza podczas pisania programu. Należy unikać sytuacji gdy jedna funkcja realizuje więcej zadań niż wynika to z nazwy.

Jest też taka “zasada”, że jeśli kopiujesz jakiś fragment kodu prawdopodobnie masz źle napisany kod i należałoby się w tym momencie zastanowić nad tym czy nie powinniśmy wyseparować jakiegoś fragmentu kodu do oddzielnej funkcji bo kopiowanie kodu jest zabójstwem dla rozwoju naszego programu. Kopiowanie kodu z jednego miejsca w drugie powinno być automatycznym alarmem dla programisty.

Wystarczy sobie wyobrazić sytuację, że ktoś poprawia nasz kod i zmienia sposób działania jakiejś pętli którą skopiowaliśmy w inne miejsce i przy zmianach w jednym miejscu należy te same zmiany wprowadzić w innym. Osoba która nie wie że ten fragment jest skopiowany prawdopodobnie będzie się zastanawiała wiele godzin dlaczego program nagle przestał działać poprawnie (a kompiluje się bez błędów) i będzie szukała problemu i traciła czas. Gdy już znajdzie to najprawdopodobniej będzie przeklinała autora takiego rozwiązania.

Do takich rzeczy użyj debuggera:

$ clang -g -O0 -o drzewo -std=c99 drzewo.c
$ ./drzewo < tekst.txt
Segmentation fault (core dumped)
$ lldb37 -f drzewo -c drzewo.core 
(lldb) target create "drzewo" --core "drzewo.core"
Core file '/home/tomek/roboczy/prog/drzewo/drzewo.core' (x86_64) was loaded.
(lldb) bt
* thread #1: tid = 0, 0x000000000040093e drzewo`kopiuj(slowo1=0x00007fffffffe430, slowo2=0x0000000000000000) + 62 at drzewo.c:28, name = 'drzewo', stop reason = signal SIGSEGV
  * frame #0: 0x000000000040093e drzewo`kopiuj(slowo1=0x00007fffffffe430, slowo2=0x0000000000000000) + 62 at drzewo.c:28
    frame #1: 0x0000000000400b2d drzewo`wstaw(slowo=0x00007fffffffe430, ptr=0x00007fffffffe470, najczestsze=0x0000000801006280, top=100) + 125 at drzewo.c:63
    frame #2: 0x0000000000400f94 drzewo`main(argc=1, argv=0x00007fffffffe508) + 276 at drzewo.c:139
    frame #3: 0x000000000040073f drzewo`_start + 367

i już widzisz że program wylatuje w funkcji kopiuj, slowo2 jest pustym wskaznikiem a ty tam próbujesz zapisać slowo1.

*ptr=malloc(sizeof(struct Node));//!!!!!!!
		(*ptr)->licznik=1;
		(*ptr)->lewy=NULL;
		(*ptr)->prawy=NULL;

brakuje alokacji pamięci dla ‘slowo’

while(najczestsze[k]->licznik < (*ptr)->licznik){k--;}

brakuje warunku stopu dla k, wyjdziesz poza tablicę

while(najczestsze[top-1]->licznik < (*ptr)->licznik){top--;}

tutaj podobnie

if(porownaj(slowo, (*ptr)->slowo)==-1){wstaw(slowo, &((*ptr)->lewy), najczestsze, top);}

jak sobie napiszesz takie długie jednolinijkowce to będzie ci trudniej ustawiać breakpointy w debuggerze, dodatkowo jak wrzucisz do repozytorium to jako diffa dostaniesz całą linijkę

void wypisz(Tree* tab, int rozmiar)
{
	for(int i=0; i<rozmiar; i++)
	{
		printf("%s %d", tab[i]->slowo, tab[i]->licznik);
	}
}

niektóre elementy tej tablicy (wskaźniki) mogą być puste

while(scanf("%49s", slowo)!=EOF)
	{
		konwersjanaslowo(slowo);
		wstaw(slowo, &ptr, najczestsze, top);
	}

w funkcji wstaw() uzywasz najczestsze[top] czyli wychodzisz poza bufor.

Poprawiony kod: http://pastebin.com/5UXvCBd1

$ clang -o drzewo -g -O0 -std=c99 -Wall -pedantic drzewo.c
$ ./drzewo < tekst.txt
ALA 2
MA 3
KOTA 2
KOT 1
ALE 2
NIE 1

 

Ta kontrukcja jest prawidłowa, zobacz jak jest zdefiniowane ptr (wskaźnik na wskaźnik).

 

Dzięki wielkie za wszystkie odpowiedzi, udało mi się to w końcu napisać. Główne problemy rzeczywiście dotyczyły alokacji pamięci na słowo. Zmodyfikowałem też trochę algorytm- już nie używam tablicy najczestsze, po prostu przebudowuję drzewo na listę uporządkowaną licznikami. Jeszcze raz wielkie dzięki.