Code review - co mogę poprawić w programie

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!

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.
1

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?

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.

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);
}
0

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

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ć :)

1

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

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.

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