Code review - co mogę poprawić w programie

Odpowiedz Nowy wątek
2019-07-11 17:24
0

Cześć, zacząłem się trochę bawić c++, a wcześniej nie miałem styczności z programowaniem. Zrobiłem prosty program, można powiedzieć taką mała mini bazę danych, która pozwala na zapisywanie i odczytywanie informacji o samochodach. Program pozwala użytkownikowi dodać informacje o aucie tzn. model, markę, rok produkcji i przebieg i później ją odczytać. Jeśli zamknie program i później go otworzy, to nadal będzie miał dostęp do poprzednio dodanych aut. Czego powinienem unikać w kodzie i co mogę w nim poprawić, bo jestem pewien, że jest tego sporo :D


#include "pch.h"
#include <iostream>
#include <string>
#include <vector>
#include <fstream>
using namespace std;
class car // creat a class 
{
private: // variables in class
    string brand;
    string model;
    int pdate;
    float car_milea;
public: // methods 
    car(string brand = "empty", string model = "empty", int pdate = 0, float car_milea = 0) // method of adding new car
    {
        this->brand = brand;
        this->model = model;
        this->pdate = pdate;
        this->car_milea = car_milea;
    }
    void c_pok() // shows information about car
    {
        cout << "The " << brand << " " << model << " was produced in " << pdate << " and has " << car_milea << "km mileage \n";

    }
};

vector<car> c_vector; // vector to keep data 

int main()
{
    string model, brand; // variables 
    int pdate;
    float car_milea;

    fstream data; // creat file to save and read 
    data.open("cardata.txt", std::ios::in);
    if (data.good() == false)
        cout << "Something went wrong!";
    else // read input if exists 
    {
        string line;
        int nr_of_line = 1;
        while (getline(data, line))
        {
            switch (nr_of_line)
            {
            case 1:
                brand = line; break;
            case 2: model = line; break;
            case 3: pdate = atoi(line.c_str()); break;
            case 4: car_milea = atof(line.c_str()); break;
            }
            if (nr_of_line == 4)
            {
                c_vector.resize(c_vector.size() + 1);
                c_vector.push_back(car(brand, model, pdate, car_milea));
                nr_of_line = 0;
            }
            nr_of_line++;

        }
        data.close();
    }
    data.open("cardata.txt", std::ios::out | std::ios::app);
    if (data.good() == false)
        cout << "Something went wrong!";
    else 
    {
        while (true) // main func
        {
            int n;
            cout << "Add new car - 1" << endl;  // add new car 
            cout << "List of cars - 2" << endl; // shows list of cars
            cout << "Exit - 3" << endl; // exit the program
            cin >> n;
            if (n > 3)continue;
            if (n == 1) // option nr 1
            {

                c_vector.resize(c_vector.size() + 1);
                cout << "Its brand: "; cin >> brand; data << brand << endl;
                cout << "Its model: "; cin >> model; data << model << endl;
                cout << "Its production year: "; cin >> pdate; data << pdate << endl;
                cout << "Its mileage "; cin >> car_milea; data << car_milea << endl;
                c_vector.push_back(car(brand, model, pdate, car_milea));
            }
            if (n == 2) // option 2
            {
                int p = c_vector.size();
                for (int w = 1; w < p; w = w + 2)
                {
                    c_vector[w].c_pok();
                    cout << endl;
                }
            }
            if (n == 3) // option 3
                break;

        }
        data.close();
    }

}

Z góry dzięki!

Pozostało 580 znaków

2019-07-11 18:27
2
  1. Można wydzielić poszczególne funkcjonalności programu do osobnych plików/klas.
  2. Wykorzystać narzędzia z nowszych wersji c++. np lista inicjalizacyjna dla konstruktora.
  3. Komentarze typu " // shows information about car" są zbędne. Nazwa funkcji ma mówić co robi. Jeżeli w klasie car będzie funkcji o nazwie toString, printObject to będzie jasne o co chodzi, c_pok już nie do końca jest jasne.
  4. cout << endl; w Twoim wypadku jest nie do końca poprawne. Lepiej '\n'.
  5. Spójrz na wzorzec RAII.

Pozostało 580 znaków

2019-07-11 18:36

Main za dużo wie o bebechach klasy, ja bym przeniósł analizę streama do metody klasy, oczywiście in-stream przekazany argumentem

Klasa za dużo wie o świecie zewnętrznym (np drukuje akurat na cout). Out-stream powinien być argumentem funkcji c_pok(), a sama nazwa funkcji lepsza.
Np zwróć uwagę, drukujesz i tak z maina <-> czujesz, że funkcja c_pok() jest zbyt sztywna, nieprzydatna?

void c_pok(ostream & st) const

Inaczej mówiąc: kod drukujący jest w dwu egzemplarzach, powinien być w jednym

c_vector.resize(c_vector.size() + 1);

jest w zasadzie zbędne, i tak by się automatycznie zresizował

pdate jako integer jest niejasne -> komentujesz błahsze rzeczy, a to nie. Raczej nie jest to data, może tylko rok?

edytowany 2x, ostatnio: AnyKtokolwiek, 2019-07-11 18:39

Pozostało 580 znaków

2019-07-11 19:42
2
c_vector.resize(c_vector.size() + 1);

jest nie tyle zbędne, co błędne.
resize() w tym przypadku zwiększa rozmiar tablicy i wstawia domyślne wartości na miejsce nowych komórek.
push_back(x) zwiększa rozmiar tablicy i wstawia element x w miejsce nowej komórki.
Jak to nie jest wystarczająco jasne, to polecam uruchomić:

void print(const vector<int> &tab)
{
    cout << tab.size() << ": ";
    for (auto i : tab) cout << i << " ";
    cout << "\n";
}

int main()
{
    vector<int> tab;
    print(tab);
    tab.resize(tab.size() + 1);
    print(tab);
    tab.push_back(-13);
    print(tab);
    tab.push_back(4);
    print(tab);
    tab.resize(tab.size() + 1);
    print(tab);
    tab.push_back(25);
    print(tab);
}

Poza tym: nie używaj zmiennej globalnej.

Pozostało 580 znaków

2019-07-11 22:14
1
  1. Bezsensowne komentarze opisujące oczywistości np: creat a class variables in class. To jest tylko szum informacyjny
  2. zmienna globalna, da się je robić, ale jest to zła praktyka,
  3. wielka funkcja main
  4. Jak pisze twonek złe użycie std::vector
  5. ten switch (nr_of_line) z pętlą jest bezsensu. Prościej było:
std::istream& car::load(std::istream& input)
{
    if (std::getline(std::getline(input, model), brand) >> pdate >> car_milea) {
        std::string dummy;
        std::getline(input, dummy).clear(); // potrzebne by skonsumować koniec linii po liczbie
    }
    return input;
}

std::vector<car> loadCars(std::istream& input)
{
    std::vector<car> result;
    car c;
    while(c.load(input)) {
        result.push_back(c);
    }
    return result;
}

std::vector<car> loadCarsFromFile(const std::string& fileName)
{
     std::ifstream f{fileName};
     return loadCars(f);
}

Jeśli chcesz pomocy, NIE pisz na priva, ale zadaj dobre pytanie na forum.
edytowany 2x, ostatnio: MarekR22, 2019-07-11 22:21

Pozostało 580 znaków

2019-07-12 00:08
0

Dzięki wszystkim za pomoc, a czy jest sens wrzucać takie programy (już po poprawie) na swojego githuba?

wrzucac na github zeby dalej rozwijac i miec historie zmian - jak najbardziej, ale wrzucac zeby sie nimi chwalic np. na rozmowie rekrutacyjnej - wg. mnie jeszcze troszke za malo funkcjonalnosci - odys 2019-07-13 23:20

Pozostało 580 znaków

2019-07-12 00:10
1

nie, to tak jakbyś wrzucał hello world tyle, że czytane z pliku i dodawane do vectora.
Podłącz się do api np. https://www.flightradar24.com/, pobierz aktualnie latające samoloty ze swojej okolicy którą wpiszesz w parametrach wywołania programu podając długość i szerokość geograficzną, informacje zapisuj do pliku, wtedy można wrzucać :)

edytowany 3x, ostatnio: au7h, 2019-07-12 00:14

Pozostało 580 znaków

2019-07-12 00:39
1

@au7h: brzmi jak dobry pomysl!
Podpowiedziałbyś może, którym bibliotekom powinienem się przyjrzeć?

ja mam taką pracę inżynierską tylko że w androidzie :P libcurl i libjson na pewno się przydadzą - au7h 2019-07-12 00:44
boost posiada wszystkie komponenty jakich bys potrzebowal, ale uzywanie ich moze byc troche hardcorowe dla poczatkujacego - odys 2019-07-13 23:18

Pozostało 580 znaków

2019-07-13 23:17
2

Nie bede duplikowal odpowiedzi o zmiennych globalnych, RAII itp. wiec napisze o nazwach, ktorych uzywasz. Np. "car_milea". Wg mnie nie ma sensu skracac w ten sposob. Na prawde nie zyszkujesz wiele odcinajac dwie literki, "car_mileage" czyta sie o wiele przyjemniej. "c_pok" jest juz kompletnie nieczytelne (mialo byc po polsku czy angielsku ? :D), co jest zlego w np. "print_info" (albo lepiej dodaj "operator << ()" dla "std::ostream"). Nazwy sa bardzo wazne, staraj sie pisac taki kod jaki chcialbys czytac za pol roku.

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

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