Uwagi oraz sugestie do działającego programu

0

Cześć, od jakiegoś czasu uczę się C, przyszedł czas na listę wskaźnikową, przy okazji jej nauki napisałem program w którym użyłem większości rzeczy których się nauczyłem więc można powiedzieć, że jest to obecnie takie podsumowanie mojej wiedzy oraz pierwszy "większy" program. Języka uczę się sam, trochę z książki, trochę ze stron, w każdym razie nie mam kogo się o coś zapytać oraz nie miałem kontaktu z dobrze napisanym kodem więc nie wiem co jest dobrym nawykiem a co złym.
Celem tego postu jest zwrócenie mi uwagi na wszystkie błędy, słownictwo czy złe nawyki. Jeżeli ktoś ma czas to dziękuję bardzo.
Chciałbym Was zapytać:

  1. Czy jest dobrze napisany?
  2. Czy jest napisany w poprawnym C? Bez czegoś z C++?
  3. Czy kod jest czytelny?
  4. Co można w nim poprawić?

Jak działa program:
Jego celem jest zapisywanie do listy liczb nieujemnych odczytanych z terminala, 'end' kończy działanie a pozostałe znaki czy tekst uznaje jako błąd i każde podać ponownie liczbę.

Wydaje mi się, że trochę namieszałem pisząc ten program więc dodam tutaj jeszcze kilka komentarzy rozjaśniających.

  • linia 21, funkcja 'sprawdz' zwraca mi zero (gdy podałem liczbę) albo jedynkę (gdy wpiszę 'end')
  • linia 57, żeby program przy wypisywaniu ostatniej liczby nie kończył przecinkiem np. (34, 54435, 425,) a (34, 54435, 425)
  • linia 64, nie wiem na co zamienić goto aby przejść do ponownego wpisywania liczb, z tego co kojarzę to goto powinno się unikać
  • linia 70, dodałem aby program sprawdzał całą odczytaną linię, wcześniej miał problem z wpisanymi jednocześnie literami i cyframi
#include <stdio.h>
#include <ctype.h>
#include <stdlib.h>
#include <string.h>

typedef struct element {
    struct element *next;
    int val;
} elementy_listy;

void dodaj(elementy_listy *lista, int *wczytana);
void wypisz(elementy_listy *lista);
int sprawdz(int *wczytana);

int main() { 
    elementy_listy *first;
    int test, *wczytana;
    wczytana = malloc(sizeof(int));

    printf("Podaj nieujemna liczbe lub wpisz komende 'end' konczaca wpisywanie: ");
    test = sprawdz(wczytana);

    if (test == 0){
        first = (elementy_listy*) malloc (sizeof(elementy_listy));
        first -> val = *wczytana;
        first -> next = NULL;
    } else { return 1; }
    
    while(test == 0){
        printf("Podaj kolejna liczbe: ");
        test = sprawdz(wczytana);
        if(test == 0){
            dodaj(first, wczytana);
        } else { wypisz(first); }
    }
    return 0; 
}

void dodaj(elementy_listy *lista, int *wczytana){
    elementy_listy *wskaznik, *nowy;
    wskaznik = lista;
    while (wskaznik -> next != NULL){
        wskaznik = wskaznik -> next;
    }
    nowy = (elementy_listy*) malloc (sizeof(elementy_listy));
    nowy -> val = *wczytana;
    nowy -> next = NULL;
    wskaznik -> next = nowy;
}

void wypisz(elementy_listy *lista){
    elementy_listy *wskaznik = lista;
    while(wskaznik -> next != NULL){
        printf("%d, ", wskaznik -> val);
        wskaznik = wskaznik -> next;
    }
    printf("%d", wskaznik -> val);
}

int sprawdz(int *wczytana){
    char napis[15], *n;
    int i = 0;

    wpisz:
    n = gets(napis);
    if( ((n[0] == 'e') || (n[0] == 'E')) && ((n[1] == 'n') || (n[1] == 'N')) && ((n[2] == 'd') || (n[2] == 'D')) && (n[3] == '\0') ){
        return 1;
    }
    i = 0;
    while(n[i] != '\0'){
        if(isdigit(n[i]) == 0){
            printf("--- Wpisz nieujemna liczbe lub 'end' ---\nPodaj kolejna liczbe: ");
            goto wpisz;
        } else { i++; }
    }
    *wczytana = atoi(n);
    return 0;
}
3

Automat mówi że coś jest źle.

Na szytbkiego:

  • twój kod kłamie. sprawdz powinno coś sprawdzać, a nie wczytywać cokolwiek.
  • widzę goto, chcesz by cię ktoś tu zamorodował? (W tym ja). Już 54 lat temu ktoś napisał Go To statment considered harmful.
  • używaj fgets a nie gets
  • castowanie z void* do innego wskaźnika jest potrzebne tylko w C++
2

Jedynym jako tako akceptowalnym użyciem goto (tylko dla C, nie dla C++) jest "goto error" by móc zwolnić zasoby i zaoszczędzić sobie powtórzeń albo głęboko zagnieżdżonych ifów. Jest to przyzwoicie wytłumaczone w tym wątku StackOverflow

Twoje użycie goto zasługuje na "pacnięcie" linijką po dłoniach.

2

Z bledow widocznych od razu bez analizowania kodu to dosyc typowe ignorowanie rezultatu malloca i [mam nadzieje] mniej typowe nie wolanie free() dla kazdego malloc() (tez po dowolnym bledzie gdziekolwiek w programie).
Z analogicznych typowych bledow to nie sprawdzasz ewentualnego bledu atoi() (to ze sie nie da nie jest zadnym usprawiedliwieniem).
Technicznie duplikacja kodu bledem nie jest, ale dla tworzenia/inicjalizowania elementy_listy powinna byc osobna funkcja.
wczytana powinno byc na stosie (choc tez poprawne allokacje/free bledem nie sa).

1

Wielkie dzięki za wszystkie uwagi, postaram się na dniach poprawić kod i wrzucę go ponownie.

0

Prawie wszystkie uwagi poprawione.

@MarekR22:

  1. Dałem nazwę sprawdź ponieważ po wczytaniu znaków sprawdza ona czy jest to liczba, jeżeli tak to zwraca mi tą liczbę. Przynajmniej mi taka nazwa wydaje się sensowna.
  2. Poprawione, miałem trudności z napisaniem programu bez goto ale wyszło, coś takiego jest dobrze czy za bardzo zagnieżdżone?
  3. Poprawione.
  4. Poprawione, szukając informacji o tym widziałem dużo przykładów z tym błędem, a nawet bardzo dużo, mam wrażenie, że na stronach z artykułami (nie forach) w ponad 50% jest ten błąd.

@several:
Poprawione, miałem trudności z napisaniem programu bez goto ale wyszło, coś takiego jest dobrze czy za bardzo zagnieżdżone?

@eleventeen:

  1. Sprawdzam rezultat malloc i dodałem free, jedynie nie jestem pewny dodanych free po błędach.
  2. Z tym atoi() to nie wiem co mam sprawdzać i jak, chodzi o to czy jeżeli przekonwertuje na 0 to jest to rzeczywiście 0 czy błąd konwersji?
  3. Nie wiem o jaką duplikacje kodu Ci chodzi.
  4. Chciałem mieć dostęp do niej w całym programie, dodałem free.

Jak teraz oceniacie kod? Jakie mam w nim jeszcze błędy i co mogę poprawić?

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

#define MAX_N 15

typedef struct element 
{
    struct element *next;
    int val;
} elementy_listy;

void dodaj(elementy_listy *lista, int *wczytana);
void wypisz(elementy_listy *lista);
int sprawdz(int *wczytana);

int main() 
{ 
    elementy_listy *first;
    int test, *wczytana;
    wczytana = malloc(sizeof *wczytana);
    if(wczytana == NULL)
    {
        printf("Pamiec niedostepna\n");
        exit(1);
    }

    printf("Podaj nieujemna liczbe lub wpisz komende 'end' konczaca wpisywanie: ");
    test = sprawdz(wczytana);

    if (test == 0)
    {
        first = malloc(sizeof(elementy_listy));
        if(first == NULL)
        {
            printf("Pamiec niedostepna\n");
            free(wczytana);
            free(first);
            exit(1);
        }
        first -> val = *wczytana;
        first -> next = NULL;
    } else { 
        return 1; 
    }
    
    while(test == 0)
    {
        printf("Podaj kolejna liczbe: ");
        test = sprawdz(wczytana);
        if(test == 0)
        {
            dodaj(first, wczytana);
        } else { 
            wypisz(first); 
        }
    }
    free(wczytana);
    free(first);
    return 0; 
}

void dodaj(elementy_listy *lista, int *wczytana)
{
    elementy_listy *wskaznik, *nowy;
    wskaznik = lista;

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

    nowy = malloc(sizeof(elementy_listy));
        if(nowy == NULL)
        {
            printf("Pamiec niedostepna\n");
            free(wczytana);
            exit(1);
        }

    nowy -> val = *wczytana;
    nowy -> next = NULL;
    wskaznik -> next = nowy;
}

void wypisz(elementy_listy *lista)
{
    elementy_listy *wskaznik = lista;

    while(wskaznik -> next != NULL)
    {
        printf("%d, ", wskaznik -> val);
        wskaznik = wskaznik -> next;
    }
    printf("%d", wskaznik -> val);
}

int sprawdz(int *wczytana)
{
    char napis[MAX_N], *n;
    int i = 0;
    int j = 0;

    while(1)
    {
        n = fgets(napis, MAX_N, stdin);
        if(n == NULL)
        {
            printf("Blad odczytu");
            free(wczytana);
            exit(1);
        }

        int dlugosc = strlen(n);
        n[dlugosc - 1] = '\0';

        i = 0;
        j = 0;

        if(((n[0] == 'e') || (n[0] == 'E')) && ((n[1] == 'n') || (n[1] == 'N')) && ((n[2] == 'd') || (n[2] == 'D')) && (n[3] == '\0'))
        {
            return 1;
        } 
        else if(isdigit(n[i]) != 0)
        { 
            i++;
            while((n[i] != '\0') && (j == 0)) 
            {
                if (isdigit(n[i]) != 0)
                {
                    i++;
                } else {
                    j = 1;
                }
            }
            if(n[i] == '\0') 
            {
                *wczytana = atoi(n);
                return 0;
            } else {
                printf("--- Wpisz nieujemna liczbe lub 'end' ---\nPodaj kolejna liczbe: ");
            }
        }  else {
            printf("--- Wpisz nieujemna liczbe lub 'end' ---\nPodaj kolejna liczbe: ");
        }
    }
}
1

Może sam siebie oceń:

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

typedef struct node
  {
   int value;
   struct node *prev,*next;
  } node;

typedef struct { node *head,*tail; } list;

node *makenode(int value,node *prev,node *next)
{
   node *tmp=(node*)malloc(sizeof(node));
   tmp->value=value;
   tmp->next=next;
   tmp->prev=prev;
   return tmp;
}

void addtail(list *lst,int value)
{
   node *tmp=makenode(value,lst->tail,NULL);
   if(lst->tail) lst->tail->next=tmp;
   else lst->head=tmp;   
   lst->tail=tmp;
}

void clean(list *lst)
{
   while(lst->head)
   {
      lst->tail=lst->head;
      lst->head=lst->head->next;
      free(lst->tail);
   }
   lst->tail=NULL;
}

void showHead(list *lst)
{
   node *i;
   printf("{ ");
   for(i=lst->head;i;i=i->next) printf("%d ",i->value);
   printf("}\n");
}

int main()
{
	list lst={NULL,NULL};
	for(;;)
	{
		printf("Podaj nieujemna liczbe lub wpisz komende 'end': ");
		char end[4];
		int value;
		if((scanf("%i%3[^\n]",&value,end)==1)&&(value>=0))
		{
			addtail(&lst,value);
			printf("dodano %u\nstan: ",value);
			showHead(&lst);
		}
		else if((scanf("%3[^\n]",end)==1)&&(!strcmp(end,"end")))
		{
			printf("Stan obecny: ");
			showHead(&lst);
			clean(&lst);
			printf("koniec, czyszczenie listy\n\n");			
		}
		else
		{
			printf("Blad wprowadzania\n");			
		}
		while(getchar()!='\n') {}
	}	
	return 0;
}
1

@MarekR22:
4) Poprawione, szukając informacji o tym widziałem dużo przykładów z tym błędem, a nawet bardzo dużo, mam wrażenie, że na stronach z artykułami (nie forach) w ponad 50% jest ten błąd.>

Niepotrzebne != bledne.

@eleventeen:

  1. Sprawdzam rezultat malloc i dodałem free, jedynie nie jestem pewny dodanych free po błędach.

Ja tez nie. Np. w dodaj() zwalniasz wczytana ktore przydzililes gdzie indziej (choc nie mam pojecia czemu ona cialge w tej wersji jest allokowana) i ciezko sie to czyta. Gdybys zwalnial pamiec w tej samej funkcji w ktorej ja przydzielasz (a choc nie zawsze, to z reguly tak powinno byc) to by bylo oczywiste kto jest odpowiedzialny za zarzadzanie pamiecia. Poza tym zdaza sie ze zwalniasz cos co przed chwila sprawdziles ze jest NULL (nie jest zle, ale jest bez sensu). Poza tym, mimo ze to bardzo czeste, to nie polecam exit(1);. Warto sie nauczyc zwracac bledy z kazdej funkcji zamiast konczyc program po kazdym bledzie. Jesli naprawde nie da sie nic innego zrobic to kod w okolicach maina powinien wychodzic po tym jak zostal zwrocony mu blad z ktorym nie moze sobie poradzic. Ale nie kazda funkcja np. wczytujaca cos dysku co zawsze moze sie nie udac.

  1. Z tym atoi() to nie wiem co mam sprawdzać i jak, chodzi o to czy jeżeli przekonwertuje na 0 to jest to rzeczywiście 0 czy błąd konwersji?

Niekoniecznie 0 musi oznaczac blad. Np. co zwroci atoi("9876543210") dla 32 bitowego inta ?

  1. Nie wiem o jaką duplikacje kodu Ci chodzi.

Dodaj jakies pole do element i zobacz w ilu miejscach musisz zmieniac. Zreszta dodajac obsluge bledow zmieniles w 2 miejscach zamiast w 1. I teraz w 2 miejscach ten kod wyglada nieco inaczej.
A dodajac obsluge bledow wprowadziles kolejne duplikacje:

if(first == NULL)
{
	printf("Pamiec niedostepna\n");
	free(wczytana);
	free(first);
	exit(1);
}
...
free(wczytana);
free(first);

Gdybys mial wiecej potencjalnych bledow i kilka zmiennych ktore trzeba posprzatac to bys to zwlanianie za kazym razem pisal?
Najprostsze (nie jedyne) rozwiazanie (choc nie raz widzialem cos podobnego jak napisales. zgaduje ze dlatego ze programisci nie uzywaja goto z powodu "bo nie". a do obslugi bledow jest to [subiektywnie] najprostsze/najczytelniejsze rozwiazanie):

  char* first = NULL;
...
  if (first == NULL) {
    goto finally;
  }
...
finally:
  free(first);

Jak teraz oceniacie kod? Jakie mam w nim jeszcze błędy i co mogę poprawić?

Zmieniles formatowanie. Dla mnie jest czytelniejsze. To jest oczywiscie subiektywne. Najwazniejsze zeby bylo spojne w calym kodzie. Nie bylo i ciagle nie jest.
Co sie stanie jak uzytkownik wpisze "1234567890987654321234567890" ?
To jest slabe: ((n[0] == 'e') || (n[0] == 'E')) && ((n[1] == 'n') || (n[1] == 'N')) && ((n[2] == 'd') || (n[2] == 'D'))
napis i n to jest to samo. Nie widze powodu dla ktorego koniecznie musza byc obydwie zmienne.

2

Może popatrz na to:
https://godbolt.org/z/Txh8T85zd

1 użytkowników online, w tym zalogowanych: 0, gości: 1