Pomoc w czystym kodzie - zadanie z kursu najblizsza sredniej.

Question

Cześć, to mój pierwszy post, zaczynam dopiero programowanie i naukę języka C++, chciałbym usłyszeć pierwsze porady co mógłbym zmienić w kodzie i co poprawić, jestem pewien, że to zadanie można zrobić o wiele lepiej i bardziej "przejrzyście". Poniżej wstawiam kod i jeżeli ktoś znajdzie chwilę mógłby wskazać miejsce w kodzie i co poprawić z góry dzięki za pomoc .

 

#include <iostream>

using namespace std;
float liczba[5];
float srednia=0;
float wartosc[5];
float bliska;
int main()
{
    cout << "Podaj 5 liczb oddzielonych spacja: " << endl;
    for (int i=0; i<5; i++) {
        cin>>liczba[i];
}
    for (int i=0; i<5; i++) {
        srednia+=liczba[i];
    }
    srednia/=5;
    cout<<"Srednia z tych liczb wynosi: "<<srednia<<endl;

    for (int i=0; i<5; i++) {
        wartosc[i]=srednia-liczba[i];
        if (wartosc[i]<0) wartosc[i]=-wartosc[i];
    }
    bliska = wartosc[0];
    for (int i=1; i<5; i++) {
        if (wartosc[i]<bliska) bliska=wartosc[i];
    }
    for (int i=0; i<5; i++) {
        if (liczba[i]==liczba[i-1]||liczba[i]==liczba[i-2]||liczba[i]==liczba[i-3]||liczba[i]==liczba[i-4]) continue;
        if (bliska==wartosc[i]) cout<<"Najblizsza sredniej jest liczba: "<<liczba[i]<<endl;

    }

    return 0;
}

Program zadziałał dobrze w każdym przypadku, jednak chciałbym go i tak udoskonalić.

Answer 1

Dzięki wszystkim za pomoc, jednak wasze kody, mimo, że są ładniejsze i czytelniejsze i na pewno mają o wiele więcej sensu, to mój kod jest lepszy pod tym względem, że działa prawidłowo. Gdy do waszego kody wpiszemy liczby "3.5 6.5 0 15 0" to wyjdzie wynik jedynie jednej liczby np 6.5, a najblizej sredniej znajduje sięzarówno 3.5 jak i 6.5 dlatego mój kod na końcu ma taką dziwną postać. Nie wiedziałem jak to rozwiązać inaczej, dzisiaj jeszcze pogłówkuje i podeśle coś.

Oczywiście zastosuję się od razu do waszych innych porad dzięki jeszcze raz 

nanautzin, TOM_CPP, mokrowski.

Answer 2

Na pewno możesz pomyśleć o zmniejszeniu ilości pętli
Musisz również pomyśleć jak zautomatyzować ostatnią pętle, w tym przypadku masz tylko 5 liczb, a co jak będziesz miał 100 czy 1000, będziesz klepał 1000 warunków?

Mógłbym ci podsunąć jak ja bym to rozwiązał, ale więcej się nauczysz jak sam pokombinujesz

Answer 3

1. Nie używaj zmiennych globalnych, czyli takich które zdefiniowane są poza ciałem funkcji main.
2. W linii 29 dane odczytywane są z poza zakresu tablicy liczba.

Całość można napisać krócej, używając mniejszej liczby zmiennych i tylko dwóch pętli.

#include <iostream>

using namespace std;

int main()
{
    const int wymiar_tablicy = 5;

    float liczba[wymiar_tablicy];
    float srednia {0};
    int bliska {0};

    cout << "Podaj " << wymiar_tablicy << " liczb oddzielonych spacja: " << endl;
    for( int i {0} ; i<wymiar_tablicy ; ++i )
    {
        cin >> liczba[i];
        srednia += liczba[i];
    }

    srednia/=wymiar_tablicy;

    cout << "Srednia z tych liczb wynosi: " << srednia <<endl;

    for( int i=1 ; i<wymiar_tablicy ; ++i )
    {
        if( abs(srednia-liczba[i])<abs(srednia-liczba[bliska]) ) bliska=i;
    }

    cout << "Najblizsza sredniej jest liczba: " << liczba[bliska] << endl;

    return 0;
}

 

Answer 4

Jeśli pisał byś to "produkcyjnie", to zapewne powinno wyglądać to tak:

#include <iostream>
#include <algorithm>
#include <numeric>
#include <cstddef>
#include <iterator>
 
using namespace std;
 
int main()
{
    constexpr static size_t WYMIAR_TABLICY = 5;
 
    float liczba[WYMIAR_TABLICY];
 
    cout << "Podaj " << WYMIAR_TABLICY << " liczb oddzielonych spacja:\n";

    copy_n(istream_iterator<float>(cin), WYMIAR_TABLICY, begin(liczba));
    auto srednia = accumulate(begin(liczba), end(liczba), 0.0) / WYMIAR_TABLICY;
    auto bliska = accumulate(begin(liczba) + 1, end(liczba), *begin(liczba), [&](const auto a, const auto b) {
		return abs(srednia - a) < abs(srednia - b) ? a: b;
    });
 
    cout << "Srednia z tych liczb wynosi: " << srednia << '\n'
    	 << "Najblizsza sredniej jest liczba: " << bliska << '\n';
}

A co do uwag w Twoim kodzie:

1. Nie stosuj "wartości magicznych". W Twoim kodzie to wartość 5. W przypadku zmiany wymagania (np. więcej danych), będzie konieczność poprawy w wielu miejscach. Stąd lepiej zdefiniować stałą i jej używać.

2. Dziel kod na funkcje jeśli nie jest to pojedyncze wywołanie np. algorytmu. Poprawia to czytelność i informuje jakie masz intencje. Np. funkcja oblicz_srednia(...) już jest lepsza niż klepanie pętli w pojedynczym spagetti kodzie. Oczywiście lepiej  używać angielskiego.

3. Nie stosuj zmiennych globalnych. To zła praktyka.

4. Nie nadużywaj std::endl. W większości wypadków, wystarcza tylko znak nowej linii '\n'.

5. W pętlach staraj się preferować pre inkrementację (++i) , ponad post inkrementację (i++).

6. Nazywaj poprawne zmienne. Np. srednia to na początku nie średnia a suma.

Comments

Mógłbyś przybliżyć punkt 5, dlaczego?

---

1. Post inkrementacja, sugeruje konieczność dostępu do poprzedniej wartości zmiennej, co w typowej pętli for, nie ma miejsca.

2. Post inkrementacja sugeruje kompilatorowi by pozostawił starą wartość w rejestrze, co może prowadzić do nieefektywnego wykorzystania zasobów CPU.

3. Niektórzy twierdzą że "dobry kompilator danej platformy powinien to zoptymalizować", problem tylko z definicją "dobry", jakiej "danej platformy" i co oznacza "powinien".

---

Tak z ciekawości, jest jakiś benchmark post i pre inkrementacji w pętli for? Nie znam żadnego kompilatora dla którego miało by to znaczenie, czy dostanie ++i, czy i++. Co oczywiście nie implikuje, że go nie ma. Sprawdziłem typy POD w pętli dla GCC 10.x i wynik przy optymalizacji O1 był taki sam, wynik rozumiem jako kod wynikowy w assemblerze.

---

A czy będzie miało znaczenie jeśli typem i będzie klasa z przeciążonymi operatorami pre i post? Jak sprawdzisz implementację przeciążenia, zorientujesz się że ma to znaczenie.

Rozważ... pytasz o rodzaj kompilatora, platformę i opierasz zdanie o "je nie znam", a na drugim końcu masz czytelność kodu i logikę :)

Osobiście spotkałem się z 3 kompilatorami które były tak proste że nie wykonywały tej optymalizacji. To były platformy embedded i DSP.

---

W mojej opinii takie rozważania mają sens właśnie tylko przy specjalnych platformach jakimi są systemy wbudowane. Przy x86, czy ARM raczej opierałbym się na gwarancjach samego języka, bo o ile wiem te dwie architektury nie zawierają specjalnej instrukcji inkrementacji, jeżeli jest inaczej, to proszę poprawić. Oczywiście dla typów nie POD, to zależy, przyznaje rację pisząc, że odnoszę się właśnie do typów podstawowych. Żeby też nie było, że walczę co jest lepsze, bo uważam, że to zależy. Po prostu warto wspomnień o tym, że obecnie kompilatory takie jak msvc i gcc na pewno to zoptymalizują dla trywialnych konstruktorów kopiujących i wyżej wspomnianych typów podstawowych.

---

A rób jak chcesz w kodzie który nie jest produkcyjny (zresztą może nie tę ale inne reguły we własnych projektach traktuję uznaniowo) :)

https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

https://www.embedded.com/pre-increment-or-post-increment-in-c-c/

Pytanie dotyczyło dobrych praktyk a nie "zróbmy jak uważamy". To ostatnie może być ustalone w danym projekcie/firmie/branży.

A co do pytania:

Przy x86, czy ARM raczej opierałbym się na gwarancjach samego języka, bo o ile wiem te dwie architektury nie zawierają specjalnej instrukcji inkrementacji, jeżeli jest inaczej, to proszę poprawić.

Oczywiście że zawierają. Prawdę mówiąc, trudno będzie Ci znaleźć jakiekolwiek ISA które nie ma inc/dec.

Po prostu warto wspomnień o tym, że obecnie kompilatory takie jak msvc i gcc na pewno to zoptymalizują dla trywialnych konstruktorów kopiujących i wyżej wspomnianych typów podstawowych.

A w pkt 3. komentarza wspomniałem :) Jeszcze mogę tylko dodać "na danym poziomie optymalizacji (jakim) lub bez".

---

Oczywiście że zawierają. Prawdę mówiąc, trudno będzie Ci znaleźć jakiekolwiek ISA które nie ma inc/dec.

To muszę doczytać o tym. Dzięki. 

---

Fakt, x86 posiada odpowiednią instrukcję 

The lodsb instruction does the equivalent of mov al,[esi] followed by inc esi.

Answer 5

Dobra poprawiłem kod wszystko działa, wydaje mi się, że wygląda dużo lepiej zostawiam do oceny i oczywiście mam jeszcze jedną prośbę. Czy mógłby ktoś pomóc stworzyć ostatni warunek w kodzie, tak aby pomijał te same liczby np gdy podam 3 3 3 3 3, program poda wszystkie te liczby jako bliskie średnie ja chciałbym, żeby podaj tylko 1 raz.

#include <iostream>
#include <cstdlib>

using namespace std;


int main()
{

    const int ilosc=5;
    float liczba[ilosc];
    float suma=0;
    float srednia;
    float bliska;

    cout << "Podaj 5 liczb oddzielonych spacja: \n";

    for (int i=0; i<ilosc; ++i)
    {
        cin>>liczba[i]; suma+=liczba[i];
    }

    srednia=suma/ilosc;

    cout<<"Srednia z tych liczb wynosi: "<<srednia<<endl;

    bliska=abs(srednia-liczba[0]);

    for (int i=1; i<ilosc; ++i)
    {
        if (abs(srednia-liczba[i])<bliska) bliska=abs(srednia-liczba[i]);
    }
    for (int i=0; i<ilosc; ++i)
    {
        if (abs(srednia-liczba[i])==bliska) cout<<"Najblizej sredniej znajduje sie liczba: "<<liczba[i]<<endl;;
    }


    return 0;
}