• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Prośba o ocenę kodu C++

VPS Starter Arubacloud
0 głosów
387 wizyt
pytanie zadane 18 lipca 2021 w Nasze projekty przez bartekr90 Nowicjusz (150 p.)

Cześć,

jestem Bartek i od jakiegoś czasu przerabiam kursy z C++ na kanale Pasja informatyki. Na końcu odcinka z wskaźnikami było zadanie, którego treść pozwoliłem sobie zmodyfikować i teraz chciałbym, aby ktoś mądrzejszy ode mnie w dziedzinie programowania ocenił moje rozwiązanie. Będę bardzo wdzięczny każdej osobie, która zechce zapoznać się z kodem i powiedzieć co jest ok, a co jest bee, z góry dziękuję :).

Treść zadania:

Napisz program znajdujący liczbę najbliższą średniej z podanego przez użytkownika zbioru liczb, jeżeli będzie kilka liczb jednakowo bliskich średniej to program ma je wszystkie wypisać. Zbiór liczb rzeczywistych, o ich ilości decyduje użytkownik.

Rozwiązanie:

#include <iostream>
#include <string>
#include <math.h>
#include <stdalign.h>

using namespace std;

int main()
{
    int ile;
    float srednia, *wskaznik;
    float *tablica = new float [ile];   // deklaracja dynamicznej tablicy w pamięci
    float *tab = new float [ile];       // deklaracja dynamicznej tablicy przechowującej wynik działania

    cout << "***********************************************"<< endl <<"Podaj ile jest liczb w twoim zbiorze: ";
    cin>> ile;
    system("cls");

    wskaznik=tablica;
    for (int i=0; i<ile; i++)           //pobieranie elementów tablicy
    {
        cout<< "podaj " <<i+1 <<". liczbe twojego zbioru: " << endl;
        cin>>*wskaznik;                  //wpisanie do szufladki 'i' w tablicy wartości
        srednia+=*wskaznik;              //sumowanie wartości z szufladek tablicy
        wskaznik++;                      //przestawienie wskaźnika na kolejną szufladkę
    }

    // for (int i=0; i<ile; i++)tablica--;//dekrementacja wskaźnika w celu ustawienia na zerowy element, by móc wypisać tablice w kolejnej pętli

    system("cls");
    cout<<"Tablica: | ";

    wskaznik=tablica;
    for (int i=0; i<ile; i++)           //wypisywanie tablicy
    {
        cout<<*wskaznik<<" | ";
        wskaznik++;
    }
    //for (int i=0; i<ile; i++)tablica--; //dekrementacja wskaźnika w celu ustawienia na zerowy element tablicy.

    srednia=srednia/ile;                //obliczanie średniej

    wskaznik=tablica;
    *tab=*wskaznik;                       //nadawanie wartości początkowej liczbie najbliżej średniej, liczba najbliżej średniej została wpisana do 0-wego elementu tablicy tab

    for (int i=0; i<ile; i++)
    {
        if (fabs(*wskaznik-srednia)<fabs(*tab-srednia)) *tab=*wskaznik;
        //porównywanie zwróconej reszty bezwzględnej z różnicy liczby obecnej, do reszty bezwzględnej z różnicy liczby najbliższej średniej

        else if ((i!=0) && (fabs(*wskaznik-srednia)==fabs(*tab-srednia))) tab[i]=*wskaznik;
        //wpisanie do nowej tablicy liczby równej liczbie najbliższej średniej

        else if (i!=0) tab[i]=NULL;
        wskaznik++;
    }
    delete [] tablica;                  //usunięcie dynamicznej tablicy z pamięci
    cout<< endl << "srednia to: "<< srednia << endl << "liczba najblizsza sredniej to: | ";

    wskaznik=tab;
    for (int i=0; i<ile; i++)           //wypisywanie tablicy liczb najbliższej średniej
    {
        if (*wskaznik!=NULL) cout<<*wskaznik<<" | ";
        wskaznik++;
    }
    delete [] tab;
    return 0;
}

 

1 odpowiedź

0 głosów
odpowiedź 19 lipca 2021 przez Whiskey_Taster Pasjonat (15,610 p.)

Okej, to może ja po kolei: 

  • deklaracja tablic jest zła

Nie chodzi o to, że konstrukcja jest zła - bo jest dobra - ale o miejsce deklaracji. Następuje ona jeszcze przed podaniem wartości zmiennej 'ile' przez użytkownika, przez co tak naprawdę nie wiesz ile elementów będzie miała tablica, bo deklaracja będzie miała miejsce wcześniej, a co za tym idzie zmienna 'ile' może mieć w sobie różne liczby - od ujemnych, do małych, aż po wielkie liczby. Ten błąd sprawia, że Twoje tablice mogą w ogóle nie powstać (jeśli zmienna 'ile' będzie o wartości ujemnej), może mieć za mało elementów, albo może być za duża - innymi słowy masz bardzo małą szansę, że będzie taka, jak powinna być. Ostatecznie przez taki mały błąd może wystąpić masa problemów, na przykład w wypadku małej liczby elementów, mniejszej od ilości podanej przez użytkownika, po prostu zaczniesz nadpisywać dane w pamięci poza obrębem Twoich tablic. Albo na przykład użytkownik poda, że wpisuje 5 liczb, a program uprzednio zadeklarował 10000 miejsc. 

  • komentarze

Jest ich zwyczajnie za dużo :) Rozumiem, że się uczysz i są to Twoje początki, dlatego też warto od razu powiedzieć sobie kilka rzeczy. Pierwszą rzeczą jest to, że nie powinno się tyle komentować. Ogółem przyjmuje się, że jeśli musisz tyle komentować, to robisz coś źle :D Kod powinien być na tyle rozbity i prosty, że komentarze powinny być zbędne. Pewnie, zdarzy się, że jest coś dziwnego lub zawiłego i trzeba wsadzić tam komentarz, żeby czytający nie główkował się godziny myśląc "co autor miał na myśli". Ale w tym wypadku nie ma sensu robić komentarzy typu "//wypisywanie tablicy" czy "//porównywanie zwróconej reszty bezwzględnej z różnicy liczby obecnej, do reszty bezwzględnej z różnicy liczby najbliższej średniej". Widzisz, jakie to długie? A widzisz, że to całkiem zbędne, bo wystarczy popatrzeć na kod? :) 

Algorytm  chyba okej, ale pewnie sam próbowałbym inaczej, choćby ze względu na ostatnią pętlę for, która wykonuje się nadmiarową ilość razy w pewnych przypadkach. 

1
komentarz 19 lipca 2021 przez tkz Nałogowiec (42,000 p.)

Ten błąd sprawia, że Twoje tablice mogą w ogóle nie powstać (jeśli zmienna 'ile' będzie o wartości ujemnej), może mieć za mało elementów, albo może być za duża - innymi słowy masz bardzo małą szansę, że będzie taka, jak powinna być.

Nie powstanie tablicy da core dumpa, co jest lepszą opcją, niż przeświadczenie, że działa dobrze. 

Ostatecznie przez taki mały błąd może wystąpić masa problemów, na przykład w wypadku małej liczby elementów, mniejszej od ilości podanej przez użytkownika, po prostu zaczniesz nadpisywać dane w pamięci poza obrębem Twoich tablic.

Nie zaczniesz. System szybko go utemperuje. 

Albo na przykład użytkownik poda, że wpisuje 5 liczb, a program uprzednio zadeklarował 10000 miejsc. 

Co da w najgorszym wypadku 0.08 megabytes. Co obecnie jest tak śmieszną ilością, że system nawet się nie zająknie. 

Co do kodu.

1. using namespace, to w praktyce zło. Szczególnie globalny. Jeżeli chcesz go używać, a odradzam, to zadeklaruj go w main'ie. Niech nie wpływa na całą przestrzeń. 

2. Zmienne deklarujemy najbliżej miejsca użycia. To nie C na litość boską. 

2.1 Zmienna - nowa linia. Nie deklarujemy kilku zmiennych w jednej linii. Głównie przez czytelność. W końcu piszesz kod dla ludzi. Kompilator sobie poradzi. 

3. Nie działaj na gołych wskaźnikach. Masz od tego [], co jest czytelniejsze i mniej błędogenne. 

4.  Polecenie system jest meh. Tworzy zbędny narzut. Dodatkowo psujesz przenośność. Ale traktuj to jako taki tip na przyszłość. Obecnie nie ma raczej znaczenia. 

5. Stwórz dodatkową zmienna "suma". Przecież "srednia", to nie średnia od początku. 

6. Nawiasy przy if'ach. Staraj się maksymalnie "więzić" kod. Ułatwia czytanie, plus nie strzelisz sobie w kolano. Programowanie defensywne rządzi.

7. Po wykonaniu delete, powinieneś przypisać do usuwanego elementu nullptr. 

8.  To polecenie "else if (i!=0) tab[i]=NULL;" nie ma kompletnie sensu. tab jest tablicą intów, nie wskaźników(preferuj nullptr nad NULL). Zrób nowy iterator, który będzie służył tylko tablicy tab. w for'ze możesz mieć więcej zmiennych. Abstrahując, że NULL będzie rzutowany na int'a, co da 0. 

9. Plus za konsekwentne używanie polskiego języka. Lepsze to, niż mieszanie dwóch. Chociaż preferuj anglojęzyczne nazwy. 

komentarz 23 lipca 2021 przez bartekr90 Nowicjusz (150 p.)

Po zapoznaniu się z uwagami zacząłem szukać informacji o nullptr, ponieważ nie wiedziałem co to jest i widzę, że muszę sobie kupić dobrą książkę odnośnie c++, ponieważ na razie bazuje tylko na kursie z yt.

W części, gdzie wpisywałem NULL do elementów tablicy chciałem otrzymać szufladkę, która nie ma wartości, ale na podstawie tego co znalazłem w google nie da się tego zrobić.

Zmieniłem kod zgodnie z uwagami i umieszczam go poniżej, dodatkowo mam parę pytań do poszczególnych podpunktów:

 

Ad. 1 Czyli zamiast namespace używać operatora zasięgu "std::"?

Ad. 3 Tego pkt niestety nie rozumiem, czyli powinienem pisać tablica[i] zamiast *tablica?

Ad. 4 Próbowałem użyć clrscr(); z conio.h, ale program się nie kompilował, googlowałem jak to naprawić, ale bezskutecznie. Czy jest jeszcze jakaś inna funkcja czyszcząca konsole? Piszę w codebloks, kompilator gcc.

Ad. 8 W tym przypadku chciałem otrzymać element tablicy który nie ma wartości, ale zrezygnowałem z tego.

Ad. 2, 5, 6, 7 Poprawiłem kod.

Proszę o ponowną ocenę kodu i z góry dziękuję.

#include <iostream>
#include <string>
#include <math.h>
#include <stdalign.h>
#include <conio.h>

int main()
{
    using namespace std;
    cout << "***********************************************"<< endl <<"Podaj ile jest liczb w twoim zbiorze: ";
    int ile;
    cin>> ile;
    system("cls");

    float *tablica = new float [ile];
    float suma;

    for (int i=0; i<ile; i++)
    {
        cout<< "podaj " <<i+1 <<". liczbe twojego zbioru: " << endl;
        cin>>*tablica;
        suma+=*tablica;
        if (i<ile-1)
            tablica++;
    }
    tablica-=(ile-1);

    system("cls");
    cout<<"Tablica: | ";

    for (int i=0; i<ile; i++)
    {
        cout<<*tablica<<" | ";
        if (i<ile-1)
            tablica++;
    }
    tablica-=(ile-1);

    float srednia;
    srednia=suma/ile;

    float *tab = new float [ile];
    float *wskaznik;
    wskaznik=tab;
    *wskaznik=*tablica;

    for (int i=0; i<ile; i++)
    {
        if (fabs(*tablica-srednia)<fabs(*wskaznik-srednia)) *wskaznik=*tablica;
        else if ((i!=0)&&(fabs(*tablica-srednia)==fabs(*wskaznik-srednia))) *tab=*tablica;
        else if (i!=0) *tab=0;

        if (i<ile-1)
        {
            tablica++;
            tab++;
        }
    }
    tablica-=(ile-1);
    tab=wskaznik;

    cout<< endl << "srednia to: "<< srednia << endl << "liczba najblizsza sredniej to: | "<<*wskaznik<<" | ";

    for (int i=1; i<ile; i++)
    {
        tab++;
        if (*tab!=0) cout<<*tab<<" | ";
    }

    delete [] tablica;
    tablica=nullptr;
    delete [] tab;
    tab=nullptr;
    wskaznik=nullptr;
    getchar();
    getchar();

    return 0;
}

 

1
komentarz 23 lipca 2021 przez tkz Nałogowiec (42,000 p.)

Nie musisz nic kupować https://www.learncpp.com/.

Ad. 1 Czyli zamiast namespace używać operatora zasięgu "std::"?

I tak, i nie. Kwestia byś wiedział gdzie możesz z niego zrezygnować. Jak dojdziesz do literałów, to zauważysz, że "musisz" go użyć, "go" w domyślnie std::coś_tam.

Ad. 3 Tego pkt niestety nie rozumiem, czyli powinienem pisać tablica[i] zamiast *tablica?

Dokładnie. 

Ad. 4 Próbowałem użyć clrscr(); z conio.h, ale program się nie kompilował, googlowałem jak to naprawić, ale bezskutecznie. Czy jest jeszcze jakaś inna funkcja czyszcząca konsole? Piszę w codebloks, kompilator gcc.

W standardzie nie. Zewnętrzne tak. Tak naprawdę wystarczy prosty define, który sprawdza na jakim systemie jesteś i podstawia cls/clear. Na marginesie define też są złe, ale nie masz innego sposobu by sprawdzić wersje systemu i coś wywalić.

#include <iostream>
#include <string>
#include <cmath> //cpp ma swoje substytuty dla bibliotek z C
//#include <stdalign.h> to jest zbedne, nie uzywasz wyrownan. Po za tym, to z C. C++ ma swoje slowa kluczowe. 
//#include <conio.h> do wywalenia, tracisz przenosnosc kodu. conio.h jest dostepny wylacznie na windowsie
 
int main()
{
    using namespace std;
    cout << std::string(42, '*')<< endl <<"Podaj ile jest liczb w twoim zbiorze: ";
          //^^^^^^^^^^^^^^^^^^^skoro masz, to korzystaj(https://www.cplusplus.com/reference/string/string/string/ 6 ctor)                       
                                 //^^^^^ warto wiedziec, ze '\n' jest szybsze. Ale czy tego potrzebujesz?
    int ile;
    cin>> ile;
    system("cls"); //https://stackoverflow.com/questions/142508/how-do-i-check-os-with-a-preprocessor-directive
 
    float *tablica = new float [ile];
    float suma=0.0; // jest smiecie z pamieci, a cos do tego dodajesz, masz UB. Zawsze inicjalizuj na starcie
 
    for (int i=0; i<ile; i++)//ciekawostka ++i kiedyś było szybszie od i++. Obecnie kompilatorowi to zwisa 
    {
        cout<< "podaj " <<i+1 <<". liczbe twojego zbioru: " << endl;
        cin>>tablica[i]; //preferuj [] nad *, czytelnosc jest wazniejsza
        suma+=tablica[i];
        //if (i<ile-1)
            //tablica++; nie mozesz tak robic!!! Inkrementowanie tablicy nie jest równe przesunieciu "komorki". Tablica to nie wskaźnik!
    }
    //tablica-=(ile-1); to samo 
 
    system("cls");
    cout<<"Tablica: | ";
 
    for (int i=0; i<ile; i++)
    {
        cout<<tablica[i]<<" | ";
        //if (i<ile-1)
            //tablica++;
    }
    //tablica-=(ile-1);
 
    float srednia=suma/ile;
    //srednia=suma/ile; nie potrzebnie
 
    float *tab = new float [ile];
    float *wskaznik;// a tutaj jest lepiej, chociaz nadal nie widze sensu
    wskaznik=tab;
    wskaznik[0]=tablica[0];//czytelniej
 
    //ponizej to samo, co wyzej jezeli chodzi o petle i tablice
    for (int i=0; i<ile; i++)
    {
        if (fabs(*tablica-srednia)<fabs(*wskaznik-srednia))
        {
            *wskaznik=*tablica;
        }
        else if ((i!=0)&&(fabs(*tablica-srednia)==fabs(*wskaznik-srednia)))
        {
            *tab=*tablica;
        }
        else if (i!=0)
        {
            *tab=0;
        }
        //nawiasy, jest czytelniej 
        if (i<(ile-1))//jest zasda, że operacje zamykamy w nawiasy, rowniez dla czytelnosci 
        {
            tablica++;
            tab++;
        }
    }
    tablica-=(ile-1);
    tab=wskaznik;
 
    cout<< endl << "srednia to: "<< srednia << endl << "liczba najblizsza sredniej to: | "<<*wskaznik<<" | ";
 
    for (int i=1; i<ile; i++)
    {
        tab++;
        if (*tab!=0) 
        {
            cout<<*tab<<" | ";
        }
        //skoro jakos sobie to wypisujesz, to rzuc okiem na iomanip, jest w standardzie
    }
 
    delete [] tablica;
    tablica=nullptr;
    delete [] tab;
    tab=nullptr;
    wskaznik=nullptr;
    getchar(); //tego tez bbym sie pozbyl, zobacz std::chrono::this_thread::slee_for uspisz sobie watek i bedzie okey. 
    getchar();
 
    return 0;
}

Tak z grubsza to tyle. 

komentarz 4 sierpnia 2021 przez bartekr90 Nowicjusz (150 p.)
Bardzo dziękuję za wszystkie porady i link do tutoriala, zapoznałem się z uwagami w komentarzach i widzę jak dużo muszę się jeszcze nauczyć, ponieważ chwilę mi zajęło załapanie o co biega z tym construct string object z linku.

Inkrementowałem tablicę, ponieważ wzorowałem się na 10 lekcji kursu Cpp na kanale pasja informatyki.
komentarz 4 sierpnia 2021 przez tkz Nałogowiec (42,000 p.)
Powszechna opinia jest zbyt pozytywna na temat wspomnianego kursu. Osobiście zostałbym przy linku, który podałem. No i praktyka.
komentarz 4 sierpnia 2021 przez Whiskey_Taster Pasjonat (15,610 p.)

@tkz​​, jak oceniasz materiał z podanej przez Ciebie strony? Od dwóch tygodni się z niego uczę i jestem zadowolony rzetelnością jak i sposobem przekazania wiedzy przez autorów strony, ale chciałbym poznać opinię kogoś z doświadczeniem. 

1
komentarz 4 sierpnia 2021 przez tkz Nałogowiec (42,000 p.)
Miałem styczność z głównym autorem owego kursu. Moim zdaniem jest jednym z lepszych, darmowych kursów do C++. Sam autor ma spore doświadczenie, co w jakiś sposób przekłada się na jakość kursu.

Widzę, że zjadłem słowo w odpowiedzi powyżej.  

Powszechna opinia nie jest zbyt pozytywna na temat wspomnianego kursu(w domyślnie kurs c++ z kanału pasja informatyki). Osobiście zostałbym przy linku, który podałem. No i praktyka.
komentarz 4 sierpnia 2021 przez Whiskey_Taster Pasjonat (15,610 p.)
Fajnie, właśnie takiej informacji potrzebowałem. Ciężko znaleźć coś porządnego, co faktycznie wprowadzi niezbędne pojęcia i poruszy ważne tematy.
komentarz 5 sierpnia 2021 przez bartekr90 Nowicjusz (150 p.)

@tkz, Czy znasz tego typu kursy z javy i SQL'a?

komentarz 5 sierpnia 2021 przez tkz Nałogowiec (42,000 p.)

@Whiskey_Taster, Oczywiście bierz wszystko z drobnym przymrużeniem oka. Wszystkie "dobre" praktyki, to kwestia względna. Dobieraj do potrzeb. Imo najwięcej się nauczysz pisząc testy. 

Niestety nie znam nic, co mógłbym polecić. 

Podobne pytania

0 głosów
1 odpowiedź 441 wizyt
pytanie zadane 15 listopada 2015 w C i C++ przez Baakoma Użytkownik (780 p.)
0 głosów
1 odpowiedź 461 wizyt
+7 głosów
5 odpowiedzi 1,962 wizyt

92,454 zapytań

141,262 odpowiedzi

319,088 komentarzy

61,854 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Akademia Sekuraka 2024 zapewnia dostęp do minimum 15 szkoleń online z bezpieczeństwa IT oraz dostęp także do materiałów z edycji Sekurak Academy z roku 2023!

Przy zakupie możecie skorzystać z kodu: pasja-akademia - użyjcie go w koszyku, a uzyskacie rabat -30% na bilety w wersji "Standard"! Więcej informacji na temat akademii 2024 znajdziecie tutaj. Dziękujemy ekipie Sekuraka za taką fajną zniżkę dla wszystkich Pasjonatów!

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...