Czytelność kodu oraz optymalizacja.

Question

Witam, wygląd mojego kodu zawsze był pewnym problemem. Od jakiegoś czasu zacząłem na to bardziej zwracać uwagę. Chcialbym was poprosić o zerknięcie na kod, który napisałem i wytknięcie tego co warto poprawić.

Zadanie pochodzi z matury - http://www.speedyshare.com/Pr7UD/inf-PR-II.pdf

Przy okazji jeśli ktoś byłby miły i zerknął, czy to co napisałem w ogóle ma jakiś sens i nie jest najmniej optymalnym rozwiązaniem na świecie - byłbym wdzięczny podwójnie :)

Kod wrzuciłem na wkleja, żeby było wygodniej : http://wklej.org/id/2424431/

Comments

Czysty kod. Robert C Martin - polecam poczytać

Answer 1

1. Niekonsekwentny styl pisania kodu: Sumacyfr(), Czy_Pierwsza(int), nawiasem mówiąc mimo, że C++ nie ma jakoś specjalnie ustandaryzowanego code style'a, to w standardzie C++ raczej spotykamy snake_case, ewentualnie dość często jest spotykany CamelCase
2. ehh, klamry w "java-style", ale spoko używasz tego jednego stylu wszędzie więc jest ok 
3. Za to: int ilosc=0, sumazp=0; w jednej linii jest niewybaczalne, chyba każdy styleguide każe rozbijać to na wiele linii
4. Pętle for i tzw "smrodek przy inkrementacji, zamiast i++ lepiej napisać ++i
5. Linia 21 if i instrukcja w jednej linii
6. Linia 45 jeszcze gorzej niż punkt wyżej bo cała konstrukcja if else w jednej linii
7. Linia 55 chyba zrobiło ci się za dużo wcięć o 1 tab
8. Dalej to mamy podobne rzeczy co wymieniłem powyżej...

Ogólnie jest bardzo dobrze (jak na twój wiek, skoro przygotowujesz się do matury), można oczywiście doczepić się jeszcze polskiego nazewnictwa, tego że przekazujesz kopie zmiennych zamiast przekazać ich adres, czasami korzystasz ze skróconej wersji operatorów, a czasami z dłuższej (a += b, a = a + b), ale to są początki.

Korzystasz z jednego stylu klamrowego, w dodatku pilnujesz wcięć więc kod jest czytelny (nie licząc linii gdzie masz wiele instrukcji w jednej linii, np if() return).

Nie rozumiem jeszcze dlaczego utworzyłeś wiele obiektów ofstream (linia 16) skoro korzystasz i tak z jednego (korzystanie z jednego jest poprawne, ale po co ci ich aż tyle, czyżby śmieciowy kod po testach?)

Co do wydajności pewnie da się to zrobić gorzej więc raczej nie jest najgorzej

Comments

Ad. 1. Nazwy funkcji powinno się pisać: sumaCyfr(). Znak podkreślenia stosuje się przy stałych, które należy nazywać wielkimi literami,
np. const int ILE_DNI_W_TYGODNIU = 7;
Natomiast wielką literą rozpoczynamy nazwy klas, struktur, ogólniej: naszych własnych typów danych. np. class Kwadrat
źródło: http://geosoft.no/development/cppstyle.html#General

Ad. 4. Nigdzie nie widziałem, żeby gdzieś było napisane, że jeden preinkrementacja jest lepsza od postinkrementacji przy pętlach for. W całym dokumencie C++ Style Guidelines jest stosowany zapis i++ i taki ja też najczęściej widuję i z pomyślnością stosuję :-)

---

Ad1 ja wspominałem jedynie o funkcjach, a nie o innych rzeczach, a to że się korzysta ze snake_case także w funkcjach to można zobaczyć choćby (masz push_back, a nie pushBack) tutaj: http://www.cplusplus.com/reference/vector/vector/

I o ile dobrze kojarzę to nie widziałem jeszcze w standardowych libach tego o czym mówisz, chociaż zdaję sobie sprawę że taki styl istnieje, sam w kodzie C++ korzystam głównie z Google Style Guide

Ad4 przy postinkrementacji tworzona jest zmienna tymczasowa, podczas przy preinkrementacji nie. Dzięki temu zapisy typu a[i++] = 7, a[++i] = 7 przypiszą 7 elementom o dwóch różnych indeksach. W pętli najprawdopodobniej kompilator i tak użyje ++i, ale nikt nie mówi że musi (np visual tego nie robi; niech ktoś mnie poprawi jeżeli się mylę): https://www.quora.com/If-the-pre-increment-x-is-faster-than-post-increment-x-why-is-it-standard-practice-to-use-post-increments-in-simple-for-loops

---

Co masz na myśli mówiąć, że przekazuje kopie nie adres? Chodzi ci o zmienną referencyjną zamiast zwykłej? Po co miałbym jej tutaj używać jeśli nie chce zmieniać oryginalnej wartości?

O obiektach ofstream zapomniałem. Stworzyłem 3 dla wszystkich 3 plików. Potem jednak sprawdziłem czy można to zrobić za pomocą jednej. Zadziałało, więc tak zostawiłem. Zapomniałem tylko o ich usunięciu.

---

Taki zapis:

void foo(int a)

kopiuje niepotrzebne zmienną, przy pojedynczym wywołaniu nie ma to specjalnego znaczenia, ale zazwyczaj wywołuje się bardzo duży funkcji i może dojść do sytuacji gdy będziesz miał masę kopii, a samo kopiowanie i przekazywanie adresu do niej jest raczej bardziej wymagające niż przekazanie adresu do zmiennej.

Czyli lepiej zastosować taki zapis:

void foo(const int& a)

Dzięki temu przekażesz nie skopiujesz zmiennej i nie pozwolisz na jej modyfikację

---

Ale jesli chciałbym coś z tą zmienną zrobić, np. potrzebowałbym z jakiegokolwiek powodu zwiększyć ją o 3, to już tego nie zrobię tak?

Oczywiscie mogę zawsze ją jako o 3 większą pokazywać, ale w wielu wywowałaniach to zbędne chyba.

np:

const int x = 3;

int w = x + 3;  i tak 10 razy

---

Po co miałbym jej tutaj używać jeśli nie chce zmieniać oryginalnej wartości?

Zazwyczaj wtedy, gdy kopiowanie obiektu jest względnie wolne. W tym przypadku masz rację, zwykłe kopiowanie inta jest OK.

Szymonie:

np visual tego nie robi

Mylisz się, to jedna z najbardziej elementarnych transformacji kodu. Nawet z wyłączonymi optymalizacjami, MSVC generuje ten sam kod dla pętli z i++ oraz ++i.

Jak wyżej napisałem, nie zgadzam się z Twoim tekstem o kopiowaniu inta. CppCoreGuidelines też się nie zgadza: http://isocpp.github.io/CppCoreGuidelines/param-passing-normal.png

---

@Szymon Siarkiewicz
Ad. 1. To prawda. W standardowej bibliotece wszędzie stosuje się zapisy wyraz_wyraz zamiast wyrazWyraz. Jednak wystarczy spojrzeć na biblioteki takie jak SFML.

Z kolei już takie Allgero stosuje wyraz_wyraz. Wszystko zależy od ustaleń zespołu. Konwencje mają to do siebie, że to tylko konwencje :-)

---

Adrianie właśnie tak czułem, ale nie miałem teraz VS pod ręką, kiedyś to sprawdzałem i nie byłem pewny kompilatora na którym to robiłem, także dzięki za poprawienie.

@Sebastian a ja właśnie mówiłem o standardowych libach, SFML nie należy do standardu.

Answer 2

Skomentuję Twój kod w oparciu o konwencję pisania kodu w C++.

Pierwsza, podstawowa zasada mówi: 1 instrukcja na jeden wiersz.
Łamiesz tę zasadę np. tutaj:

if (Czy_Super_B_Pierwsza(i)) Zak1 << i << endl;

Należałoby to zapisać tak:

if (Czy_Super_B_Pierwsza(i)) 
    Zak1 << i << endl;

Tutaj tak samo:

if (Czy_Pierwsza(sumazp)) cout << "TAK"; else cout << "NIE" << endl;

Wygląda to... nieładnie. Powinno być:

if (Czy_Pierwsza(sumazp)) 
    cout << "TAK"; 
else 
    cout << "NIE" << endl;

Źródło z konwencji 
(punkt 63): http://geosoft.no/development/cppstyle.html#Conditionals

To samo tyczy się pętli. Zamiast czegoś takiego:

for (int i = 2; i <= sqrt(liczba); i++) if (liczba%i == 0) return 0;

Powinieneś zapisać:

for (int i = 2; i <= sqrt(liczba); i++) 
    if (liczba%i == 0) 
        return 0;

Kolejna rzecz, to niepoprawne nazewnictwo funkcji. Pomijam fakt, że według konwencji powinny nazywać się po angielsku. Ale nazwy funkcji rozpoczynamy małą literą. Czyli powinieneś zapisać:

bool czyPierwsza( int );

I tak każdą funkcję. Zmienne również piszemy małymi literami, a poszczególne słowa składające się na nazwę rozpoczynamy wielkimi (tak jak w przykładzie powyżej). Stałe natomiast piszemy WIELKIMI_LITERAMI.

Poza tym wszystko ładnie. Dbasz o odpowiednie wcięcia :-)
Osobiście takiego stylu klamerek nie preferuję, ale obydwa zapisy są oczywiście poprawne. Nie ma tutaj rozróżnienia na języki.
Po prostu zapis:

if( warunek )
{

}

Jest popularniejszy w językach z rodziny C, a ten który Ty stosujesz jest popularniejszy w Java, JavaScript czy CSS.

Pozdrawiam.

Comments

Dzięki, zwrócę uwagę na to by jedna instrukcja była w jednej linii.

Co zabawne, wciąż ludzie patrząc na moje klamerki pytają, czy dopiero co nie przesiadłem się z javy, kiedy w javie nigdy w życiu nie pisałem a C++ jest moim pierwszym językiem :D

---

Jest popularniejszy w językach z rodziny C, a ten który Ty stosujesz jest popularniejszy w Java, JavaScript czy CSS.

...nie? Głupoty pleciesz. Taki styl:

if( warunek ) {
 
}

Jest używany w:

• K&R
• "The C++ Programming Language" Strostrupa
• Standardzie C++
• CppCoreGuidelines
• Kernelu Linuxa
• Clangu

(wyjątkiem jest klamra bez nowej linii w definicji funkcji, która faktycznie jest typowo Javowa - ale Twój przykład z if'em jest zdecydowanie błędny.)

---

Przeważnie jeśli ktoś stosuje taki zapis przy funkcjach:

void foo()
{

}

to robi tak samo przy instrukcjach sterujących. Przeglądając mnóstwo kodów stwierdziłem, że w C, C++ i C# przeważają zapisy z klamrą od nowego wiersza.

Popularność tego zapisu, o którym pisałem, nie ma oczywiście nic wspólnego z książkami i dokumentami poświęconymi składni języka i tego, która opcja jest jedyna słuszna, prawidłowa.

Jeśli kierownik projektu zadecydował, że w kodzie ma funkcjonować taki styl, to wszyscy programiści bez względu na preferencję będą tak pisać. Trzeba to zawsze jakoś ustandaryzować i dlatego fakt, że Kernel Linuxa ma taki, a nie inny styl może wynikać z jednej decyzji, jednego człowieka, która później przerodziła się w standard. 

Podałeś mocne przykłady, ale to przecież nie są wyznaczniki standardu poprawności. Pisałem o popularności. Mógłbym tutaj przytoczyć również kilka przykładów jak chociażby cała dokumentacja języka C++ czy C# ze strony Microsoftu, która stosuje zapis klamer od nowych wierszy zarówno przy funkcjach jak i warunkach czy pętlach. 
Wszystko zależy od wizji firmy.

Stroustrup to oczywiście wielki człowiek. Jego wkład w rozwój programowania jest niebywale ogromny. Jednak styl klamer to rzecz, której nawet on nie ustandaryzuje. 

Powtórzę jeszcze, że pisałem o popularności (mając na myśli internet). IMO popularność nie jest wyznacznikiem standardu i poprawności rzecz jasna :-)
To było oczywiście oparte tylko na własnych obserwacjach. W głównej mierze przeglądając programy na forach, np. StackOverflow.

---

Oczywiście, zgadzam się że najważniejsza jest konsekwencja. Osobiście bardzo, bardzo mało mnie obchodzi kto jaki styl używa. Chciałem się tylko sprzeciwić stwierdzeniu o popularności, które uważam za nawet jeśli nie niepoprawne, to potencjalnie wprowadzające w błąd.