code review c++

Question

Witam. Napisałem aplikacje do przeliczania jednostek. Chciałbym was prosić o code review. Napiszcie co zrobiłem dobrze oraz co zrobiłem źle i co można by było robić lepiej. Z góry bardzo dziękuje. Na dole link do GitHub

https://github.com/Tymson122/matemax_repository

Answer 1

1) nazywasz zmienne po polsku a nie po angielsku

2) generalnie jakość kodu nie jest zbyt dobra :(

pierwszy z brzegu przykład:

        switch (nr_linii)
        {
        case 1: cout << linia << endl; break;
        case 2: cout << linia << endl; break;
        case 3: cout << linia << endl; break;
        case 4: cout << linia << endl; break;
        case 5: cout << linia << endl; break;
        case 6: cout << linia << endl; break;
        case 7: cout << linia << endl; break;
        case 8: cout << linia << endl; break;
        }

po co jest tu ten switch? Każdy case robi to samo.

Kolejna rzecz to masz za dużo else-ifów

Comments

Co mogę jeszcze zmienić?

Answer 2

Odnośnie klasy (pomijam wspomnianego w innej odpowiedzi switcha).

Skoro nazwa metody oznacza jednostkę wartości wejściowej, to po co mimo to podajesz i sprawdzasz jednostkę (parametr a) w kolejnych metodach? Przecież robisz to już w main? Dodatkowo dla każdej metody tworzysz oddzielny obiekt o nazwie podobnej do nazwy metody, którą chcesz na rzecz tego obiektu wywołać (o_centymetry ⇒ centymetry). Po co, przecież wystarczy jeden obiekt?

Answer 3

1. Sprawdzanie czy plik został otwarty:

       ifstream plik("menu.txt");
       if (not plik.is_open()) 
       {
           cerr << "Nie mozna odnalesc pliku z menu!";
           return 1;
       }
   

2. Wczytywanie z pliku:

   string line; 
   while (getline(plik, line)) 
       cout << line << "\n";

   lub

   cout << plik.rdbuf(); 
   

3. Używaj tylko języka angielskiego do nazewnictwa kodu.
4. Nie używaj using namespace std
5. Nazwa klasy powinna zgadzać się z nazwą pliku (ułatwia orientację w większym projekcie), 
6. Don't repeat yourself. Cała klasa obliczanie to jedna wielka drabinka ifów powtarzająca te same operacje. Dodanie kolejnej jednostki wymaga sporo wysiłku. 
   Możesz zmapować przeliczniki między jednostkami:

       std::map<string, float> factors {
           {"m", 1},
           {"cm", 0.01},
           {"mi", 0.000621371192},
           {"ca", 39.37},
           {"km", 1000}
       };

   Przykładowa funkcja konwertująca jednostki (zamiana najpierw na metry, później na jednostkę docelową) bez obsługi błędów:

   float convert(float value, string const& unit1, string const& unit2) {
       return value * factors[unit1] / factors[unit2];
   }
   

Comments

Dlaczego nie używać using namespace std?

---

Problem leży z kolizją nazw:
https://forum.pasja-informatyki.pl/266789/using-namespace-std-vs-std

---

@tymek112, możesz używać dyrektywy using namespace std, tylko ograniczaj zasięg jej działania. Czyli nie w nagłówkach, bo otwierasz przestrzeń nazw std wszędzie tam, gdzie załączysz nagłówek z takim wywołaniem, co generalnie nie jest dobre. W plikach źródłowych może być, bo ograniczasz zasięg do danej jednostki kompilacji. Możesz użyć wewnątrz funkcji - takie lokalne otwarcie przestrzeni może poprawić czytelność kodu, a i nie narobi burdelu z nazwami w innych funkcjach czy plikach.