Ocena kodu

Witam
Prosił bym o wytknięcie mi błędów w tym kodzie, jest to kod na zaliczenie.

http://pastebin.com/BH7tsYeJ

1.Samochod::licznik nie powinien być dekrementowany przy usuwaniu instancji klasy?
2.stan powinien być raczej enumem.
3.Zamiast 4 zmiennych samochod, lepiej byłoby skorzystać z tablicy.
Reszta jako-tako wygląda dobrze.

Na szybko czepiłbym się kilku rzeczy:

1. Zły include, zamiast cstring powinno być string.
2. using namespace std na samym początku to nienajlepsza praktyka, lepiej wprost odnosić się do przestrzeni albo dawać using lokalnie w metodach i funkcjach, najlepiej tylko tych elementów, które są wykorzystane (np. using std::cout).
3. Metoda wpisz() jest dość nieszczęsna. To powinna być osobna funkcja.
4. Używaj list inicjalizacyjnych konstruktora zamiast jawnego przepisywania pól.
5. Mogę się mylić co do założeń, ale w destruktorze powinieneś chyba zmniejszać licznik.
6. Co robi metoda funkcja()? I dlaczego nie nazywa się tak, żeby było to wiadome?
7. Zatrzymanie programu przez system("pause") nie jest najlepszą praktyką. W ogóle jak dla mnie zatrzymywanie programów konsolowych mija się z celem. Ale jeśli już musisz to zrobić, to użyj np. cin.get(). To pozwoli też wywalić #include <cstdlib>, który do niczego więcej nie jest tu potrzebny.
8. Lepiej byłoby podzielić to na dwie jednostki kompilacji, czyli stworzyć plik samochod.h z deklaracją klasy, samochod.cpp z definicjami metod oraz main.cpp jako program główny.

Edit Jak napisał @Patryk27 – stan pasowałby bardziej jako enum. Ponadto – czym jest konstruktor konwertujący i jak działa? Jak widzę jest to po prostu wczytanie wartości od usera z konkretną wartością indeksu. Spora niekonsekwencja: albo przez konstruktor podaje się parametry, a klasa dba o wartość indeks, albo podaje się wartość indeks, a klasa sama wczytuje dane od użytkownika (fuj). Do tego nie zmieniasz tu wartości licznik, więc po zrobieniu:

Samochod s1(1);
Samochod s2("Foo", "bar", 123); // s1.indeks == s2.indeks

Postaram się zastosować do waszych uwag, oczywiście jeżeli ktoś ma coś jeszcze do powiedzenia to proszę.

Zacząć należy od tego, że metoda wpisz nie powinna w ogóle wyglądać tak jak wygląda aktualnie. Odnosi się ona jedynie do konsoli, a co jeśli kiedyś przyjdzie czas na przeniesienie do GUI? Będzie trzeba przepisać metodę.

Cała metoda do wyrzucenia.
Powinna ona być poza klasą.

To samo z metodą wypisz. Metoda wypisz powinna zwracać stringa i nazwę powinna mieć "GetInfoCar".
W tej postaci, której jest, klasa ta kompletnie nie nadaje się do niczego. Jest napisana na "tu i teraz", a to bardzo poważny błąd (ja wiem, że na studiach takie cos przejdzie, ale... u mnie by nie przeszło)

Ogólnie bardzo słabo rozumiesz na czym polega obiektowość i klasy. To co ja napisałem ktoś może uznać za "czepianie się", bo "przecież działa", ale prosisz o ocenę kodu, więc go oceniam. Klasa do wyrzucenia, a konkretnie do przepisania.
Co do logiki samych metod to nie wnikam w nie. Są dość krótkie, więc ok.

Widać, że to poziom podstawowy i dopiero zaczynasz, wręcz raczkujesz, dlatego nie zrażaj się. Tak pisze każdy początkujący i to jest absolutnie normalne. Staraj się jednak za każdym razem pisać lepiej.

nie żebym się czepiał, ale po kiego grzyba pisać:

Samochod samochod2=Samochod ("Fiat","zly",1993);

jak można napisać po prostu:

Samochod samochod2("Fiat","zly",1993);

Te intrukcje są równoważne a mniej pisania jest przy tym.
a i używanie system pause w celu zatrzymania programu nie jest najlepszą praktyką

Te instrukcje nie muszą być równoważne. Dodatkowo zapis 2 powinien być IMHO jedynym dozwolonym, by nie było dwuznaczności przy zapisie:

JakasKlasa f();

Co to jest? Utworzenie obiektu f przy pomocy konstruktora bezparametrowego czy deklaracja funkcji zwracającej JakasKlasa i nie przyjmującej argumentów.

winerfresh napisał(a):

Te instrukcje nie muszą być równoważne. Dodatkowo zapis 2 powinien być IMHO jedynym dozwolonym, by nie było dwuznaczności przy zapisie:

JakasKlasa f();

Co to jest? Utworzenie obiektu f przy pomocy konstruktora bezparametrowego czy deklaracja funkcji zwracającej JakasKlasa i nie przyjmującej argumentów.

To jest deklaracja funkcji, bo tak mówi standard C++. W skrócie: wszystko co da się zinterpretować jako deklarację funkcji zostanie tak zinterpretowane.

Dwuznaczność widzą tylko ludzie, dla kompilatora to oczywiste. ;-)