Proszę o ocenę kodu źródłowego implementacji listy jednokierunkowej

Witajcie! Postanowiłem zmierzyć się ze strukturami danych. Nie jestem kompletnym żółtodziobem, ponieważ w liceum miałem styczność z programowaniem. Nie przykładałem się jednak i ostrzegam że mój kod jest dość niechlujny - nigdy nie nauczyłem się standardowych nawyków w formatowaniu i komentowaniu kodu (co mam nadzieję w niedalekiej przyszłości nadrobić).
Ale do rzeczy. Zamieszczam kod implementacji najprostszej listy jednokierunkowej mojego autorstwa. Póki co napisałem funkcję wyświetlającą zawartość listy oraz dodającą na jej końcu kolejny element.
Proszę o opinię i konstruktywną krytykę, ponieważ zamierzam w ramach nauki także napisać implementacje innych operacji na tej liście. Jeżeli w moim rozumowaniu są luki i błędy proszę mi je bezlitośnie wytknąć. Dziękuję z góry za konstruktywną krytykę.

struct element_listy
{
  float dana; // przykładowa wartość
  element_listy *next;  // wskaźnik na następny element
};

void PokazListe(element_listy* &poczatek)//dostaję wskaźnik na pierwszy element
{
	if (poczatek->next == NULL) std::cout << "Lista jest pusta!"; //jeżeli element jest pusty to wypisuję że lista pusta
	else //jeżeli nie jest pusty to
    {
		element_listy* temp = poczatek; // kopiuję element to zmiennej tymczasowej
		while (temp != NULL)//dopóki nie trafię na ostatni element
		{
			std::cout << temp->dana<< std::endl;//wypisuję dane
			temp = temp->next;//przypisuję do zmiennej tymczasowej kolejny element
		}
    }
}

void DodajElement(element_listy* &poczatek, float &data)//dodawanie elementu na końcu listy
{
	element_listy* nowy = new element_listy;//tworzę nowy element listy
	nowy->next = NULL; //będzie ostatni więc ustawiam mu null na wskaźniku next
	nowy->dana = data;//ustawiam mu jakąśtam wartość
	element_listy* wskaznik = poczatek;//tworzę wskaznik tymczasowy wskazujący na kolejne elementy listy i ustawiam go na pierwszym elemencie
	while(wskaznik->next != NULL)//dopóki nie trafię na ostatni element
	{
		wskaznik = wskaznik->next;//przechodzę na następny element tj. przypisuję jego adres do tymczasowego wskażnika
	}
	wskaznik = nowy; //jeżeli wskaźnik jest pusty to przypisuję mu wcześniej utworzony nowy element
} 

Jeżeli chodzi o czytelność kodu to zdecydowanie za dużo komentarzy. W sumie nigdzie tu komentarz nie jest potrzebny.
Kod powinien się sam dokumentować - tzn. nazwy zmiennych, funkcji, klas itd. powinny wskazywać na ich przeznaczenie. Komentarze są potrzebne w miejscach, gdzie na pierwszy rzut oka nie wiadomo co kod dokładnie robi.

Jeżeli chodzi o formatowanie to nie jest tragicznie, aczkolwiek wcięcia są niekonsekwentne. Jakiego IDE używasz? Mi to wygląda na Dev-C++ - jeżeli tak to go z miejsca wywal i zainstaluj np. Code::Blocks

Jeżeli chodzi o sam kod to typy argumentów są nie do końca sensowne.
Do funkcji PokazListe przekazujesz referencję do wskaźnika, co sugeruje, że to na co on pokazuje jak i sam wskaźnik może być zmienione, a nie taki jest cel tej funkcji. Powinien być raczej

const element_listy* poczatek

a jeszcze lepiej jak by był const element listy * const poczatek //stały wskaźnik do stałego elementu listy

Do funkcji DodajElement przekazujesz też referencję do wskaźnika i to byłoby ok, gdyby ta funkcja poradziła sobie, jeżeli przekazałbyś tam wskaźnik pokazujący na NULL, a w tym wypadku w najlepszym przypadku program Ci się wysypie z powodu sięgania do pamięci, której nie zaalokowałeś. Przekazywanie data przez referencję też nie jest dobre, raczej powinno być const float.
Zamiast float lepiej używać double, dzisiejsze maszyny mają wystarczająco dużo pamięci. Float był wprowadzony raczej ze względu na oszczędność pamięci, bo do obliczeń nie specjalnie się nadaje(ze względu na błąd). Double też nie jest dokładny, ale zdecydowanie lepszy niż float.

Poza tym wszystko ok.

void PokazListe(const element_listy *lista)
{
	if (lista == NULL) {
		std::cout << "Lista jest pusta!";
		return;
	}

	for (; list != NULL; lista = lista->next) {
		std::cout << lista->dana << std::endl;
	}
}


void DodajElement(element_listy* &lista, float data)
{
	element_listy **gdzie = &lista;
	while (*gdzie != NULL) gdzie = &(*gdzie)->next;

	*gdzie = new element_listy;
	(*gdzie)->dana = data;
	(*gdzie)->next = NULL;
}

Dziękuję bardzo za odpowiedzi. Teraz program działa bez zarzutu. Mieliście rację, w funkcji wyświetlającej listę brakowało prostego zabezpieczenia przed przekazaniem pustego elementu. Nie rozumiem do końca działania rozwiązania z dodawaniem ostatniego elementu.

Poprzednia wersja:

 
void DodajElement(element_listy* &poczatek, float &data)
{
        element_listy* nowy = new element_listy;
        nowy->next = NULL; 
        nowy->dana = data;
        element_listy* wskaznik = poczatek; //rozumiem, że w tym miejscu zamiast ustawiać wskaźnik na adres elementu ustawiam wskaźnik na element, i to jest błąd?
        while(wskaznik->next != NULL)
        {
                wskaznik = wskaznik->next;
        }
        wskaznik = nowy; 
} 

 
void DodajElement(element_listy* &poczatek, const double data)//dodawanie elementu na końcu listy
{
	element_listy** wskaznik = &poczatek;//tutaj wskaźnik wskazuje na "wskaźnik na element". Więc wskazuje na adres elementu a nie na jego samego?
	while(*wskaznik != NULL)//dopóki nie trafię na ostatni element
	{
		wskaznik = &(*wskaznik)->next;//przechodzę na następny element tj. przypisuję jego adres do tymczasowego wskażnika
	}
	*wskaznik = new element_listy; 
	(*wskaznik)->dana = data;
	(*wskaznik)->next = NULL;
// ten zapis (*gdzie) jest tożsamy z operowaniem na zmiennej bezpośrednio, ale dostęp uzyskuję za pomocą wskaźnika?
}

Dzięki jeszcze raz za poprawny kod Adf88. Mam nadzieję że dobrze zrozumiałem jego działanie.

Wydaje mi się, że zrozumiałeś dobrze. Chodzi o wskazanie wskaźnika :)

Kiedy na liście jest przynajmniej jeden element to wstawienie polega na wyszukaniu ostatniego elementu:

        while(wskaznik->next != NULL)
        {
                wskaznik = wskaznik->next;
        }

i dołączeniu pod wskaźnik next nowego elementu:

        wskaznik->next = nowy;

A co jeśli lista jest pusta? Wtedy trzeba dać jej nowy początek! poczatek = nowy;

Składając wszystko do kupy wyglądałoby to tak:
```cpp
if (poczatek == NULL) {
    poczatek = nowy;
} else {
    wskaznik = poczatek;
    while (wskaznik->next != NULL) wskaznik = wskaznik->next;
    wksaznik->next = nowy;
}

No i może faktycznie dla początkującego powyższa forma jest bardziej przejrzysta i zrozumiała.
Ja natomiast zastosowałem uproszczenie (które de-facto może być uznane za swoisty hak, tudzież uznane całkiem przeciwnie za utrudnienie, kwesta gustu). Zamiast rozgraniczać się na dwa przypadki (lista pust/niepusta) zastosowałem podwójny wskaźnik. Wskaźnik ten wskazuje miejsce pod które należy podłączyć nowy element.

Na początku wskazuje on na wskaźnik głowy listy:

element_listy **gdzie = &lista;

. Jeśli lista jest niepusta to gdzie będzie "przeskakiwać" do przodu na kolejne wskaźniki next dopóki nie dojdziemy do końca:gdzie = &element->next;

Warto zauważyć jedną rzeczy. Nie ważne, czy `gdzie` wskazuje na wskaźnik głowy czy na wskaźnik `next`. Wskazywany wskaźnik będzie albo `NULL` - wtedy jesteśmy na końcu i możemy podmienić owy `NULL` nowym elementem:<code class="cpp">*gdzie = nowy;

albo nie NULL - wtedy wskazywany jest element listy i możemy "iść do przodu":nastepny = *gdzie;
gdzie = &(nastepny->next);

co można uprościć do:
```cpp
gdzie = &(*gdzie)->next;

Co do tego fragmentu

*wskaznik = new element_listy;
(*wskaznik)->dana = data;
(*wskaznik)->next = NULL;
to analogicznie można by go zapisać tak:

        nowy = new element_listy; 
        nowy->dana = data;
        nowy->next = NULL;
        *wskaznik = nowy;

Aha, jeszcze dwie rzeczy.

Zamień float &data na float data. Używając referencji nie będziesz mógł przekazać stałej:

dodaj(3); // źle, błąd

ani obiektu ulotnego (czyli np. wyniku dodawania):

dodaj(x + y); // źle, błąd

Jedynie konkretną zmienną:

int x = 3;
dodaj(x); // ok

W dodatku sygnalizujesz w ten sposób, że funkcja zmieni wartość zmiennej x zwracając jakiś wynik.
Ktoś by mógł powiedzieć "no dobra, ale dlaczego nie const float &data ?". Faktycznie, gdyby przekazać dane przez const-referencję wszystkie trzy przypadki powyżej działały by jak należy. Z tym, że wydajnościowo jest to gorsze niż zwykły float data. Zazwyczaj zyskamy dopiero wtedy, kiedy przekazywana dana będzie mieć większy rozmiar np. zamiast float będzie przekazywana struktura składająca się z 4 float'ów:

struct dana { float tab[4]; };

void funkcja(const dana &d) { // referencja jest tutaj OK, zazwyczaj zyskamy wydajnościowo

Druga sprawa. Zamień const float data na float data (tu się nie zgadzam z sugestią byku_guzio). Dla wywołującego funkcję nie ma to żadnego znaczenia, i tak przekazywana jest kopia obiektu. Tak więc czy jakąś zmienną oznaczymy const lub nie ma znaczenie tylko i wyłącznie dla wnętrza samej funkcji. Gdyby funkcja była długa/zawiała/trudna i jakiś jej fragment oczekiwałby iż wartość się nie zmieni, dodatkowo byłaby możliwość przypadkowej zmiany tej wartości w innym fragmencie funkcji to wtedy const mogłoby się przydać. Obrazowo różnica jest taka sama jak przy zmiennych lokalnych. Piszemy albo:

void funkcja()
{
   int x = jakies_tam_obliczenia();
   // ...
}

albo

void funkcja()
{
   const int x = jakies_tam_obliczenia();
   // ...
}

Uważam, że powinno się dodawać const jeśli ma to na prawdę znaczenie, a nie z automatu.

Wielkie dzięki jeszcze raz. Dzięki twoim informacjom nie miałem problemu z napisaniem także wstawiania na początek i usuwania z początku i końca. Kiedy skończę sesję zabieram się za sortowania tego całego stuffu;) Pozdrawiam serdecznie!