Prośba o code review

0

Witam,
zwracam się z prośbą o inspekcję kodu. Głównie zależy mi tym, czy poprawnie używam funkcji wirtualnych i dziedziczenia, bo to, że "działa" nie znaczy, że wszystko gra w kodzie.

0

wez to wrzuc na jakiegos githuba, nikt nie bedzie zipow sciagal

0

TrainingFieldCpp

Klasa Company nie jest jeszcze ruszona, bo chciałem się upewnić, że nie będę powielać błędów.

2

Na pewno muszą być destruktory wirtualne, żeby kompilator wiedział jak sprzątać po klasach.

W konstruktorach korzystaj z listy inicjalizacyjnej.

class Base
{
   // field1
   // field2
public:
   Base(arg1, arg2)
   : field1{ arg1}, field2{ arg2} 
   {
   }
   virtual ~Base(){}
};

W klasie dzidziczącej wykorzystaj konstruktor klasy bazowej do inicjalizacji dziedziczonej części obiektu.

class Foo : public Base
{
   // fieldFoo1
   // fieldFoo2
public:
   Foo(arg1, arg2, arg3, arg4)
   :Base{arg1, arg2}, fieldFoo1{ arg3 }, 
    fieldFoo2{ arg4 } 
   {
   }
   virtual ~Foo(){}
};

Co do użycia funkcji wirtualnych, to wszystkie mogą być takie. C++ pozwala używać wirtualnych, gdy nawet nie są potrzebne. Konieczne jest opatrzenie funkcji slowem virtual tych funkcji składowych klasy bazowej, które w klasie pochodnej muszą być nadpisane, aby kompilator wiedział co z tym zrobić.

Powiedz coś więcej o projekcie. Jakie są założenia?

0

Założeniem projektu jest głównie przećwiczenie przeciążania operatorów i poprawne korzystanie z dziedziczenia.

Aby to potrenować, że zrobię klasę bazową Contact i po niej będą dziedziczyć klasa Person i Company. W ten sposób powstałoby coś na kształt książki telefonicznej. Przy okazji zdecydowałem się na użycie vectora do przechowywania kontaktów. Zrezygnowałem z jakiegokolwiek GUI, aby skupić się na pracy ze standardem.

1
  • Warto używać słowa override (od C++11). [edit: Ale widzę, że używasz... :)]
  • Po co w person.h dwie deklaracje tego samego (linie 36 i 39)?
  • Dlaczego Person dziedziczy po Contact? Jaki ma to sens i czy to modeluje relację 'jest'? To jest -- czy każda osoba jest kontaktem (w Twoim projekcie i ewentualnym szerszym kontekście)? [edit: Widzę, że na to odpowiedziałeś w poście wyżej... :)]
  • Co możesz więcej opowiedzieć o sensie metody newContact? Bo ja nie do końca rozumiem jej ideę...
0
koszalek-opalek napisał(a):
  • Warto używać słowa override (od C++11). [edit: Ale widzę, że używasz... :)]
  • Po co w person.h dwie deklaracje tego samego (linie 36 i 39)?

Chyba był problem z kompilacją, albo podłapłem to na stackoverflow. Jak wrócę do domu, to sprawdzę.

  • Co możesz więcej opowiedzieć o sensie metody newContact? Bo ja nie do końca rozumiem jej ideę...

Po prostu dodawanie nowego kontaktu do kontenera. Trochę takie "podawaliny" pod interfejs. Planowałem zrobić dodatkową klasę, która by obsłużyła "formularz" w konsoli dodania nowego kontaktu. Nazwa została Conact, bo to była pierwotnie też metoda virtualna, ale w praktyce wyszło, że jak używam więcej parametrów to już nie nadpisuję jej tylko zasłaniam nazwę(?)

2
BartoSAS napisał(a):

ale w praktyce wyszło, że jak używam więcej parametrów to już nie nadpisuję jej tylko zasłaniam nazwę(?)

Nie rób tak. Albo rób metodę wirtualną, albo metody o różnych nazwach...

Dlaczego -- "Sposób 37. Nigdy nie definiuj ponownie dziedziczonych funkcji niewirtualnych" tutaj: https://helion.pl/ksiazki/c-50-efektywnych-sposobow-na-udoskonalenie-twoich-programow-scott-meyers,cp50sp.htm#format/d
W skrócie: albo funkcja ma inną odpowiedzialność/przeznaczenie -- i wtedy inaczej się nazywa -- albo ma okreslony interfejs i stałą odpowiedzialnośc -- wtedy powinna być wirtualna...

0

W porządku. To jeszcze takie pyanie jedno - jak nie mam w klasie wskaźników, czy tam tablicy, co ma sprzątać destruktor? Bo w przykładach, które widziałem to są albo wskaźniki usuwane w destruktorze albo jest przykład pokroju klasy Słowo gdzie jest tablica typu char i ona jest czyszczona w destruktorze.

1
BartoSAS napisał(a):

W porządku. To jeszcze takie pyanie jedno - jak nie mam w klasie wskaźników, czy tam tablicy, co ma sprzątać destruktor? Bo w przykładach, które widziałem to są albo wskaźniki usuwane w destruktorze albo jest przykład pokroju klasy Słowo gdzie jest tablica typu char i ona jest czyszczona w destruktorze.

Nie wiem, co znaczy 'sprzątać destruktor'. :)

Ale jeśli chodzi o destruktor, który nie ma nic do roboty (bo nie ma wskaźników i tym podobnych rzeczy), to potrzebny jest nic nierobiący destruktor wirtualny (virtual ~Klasa() {}), jeśli po klasie ma być dziedziczenie (czyli jesli nie jest finalna) -- żeby klasa wiedziała, że ma sprzątać po odpowiedniej klasie (potomnej), niekoniecznie po typie wskaźnika/referencji. Dobrze jest tworzyć destruktory wirtualne zawsze.

To Cię jeszcze raz odeślę do ksiązki, którą wspomniałem (polecam, nie jestem od wydawcy :)) -- "Sposób 14. Umieszczaj w klasach bazowych wirtualne destruktory"

0

Wielkie dzięki - lektura już dawno zakupiona i używana, ale nie zawsze zdawałem sobie sprawę, że jakiś punkt mnie dotyczy :p Ale największy mętlik robią mi te punkty o rozważaniu sposobu przekazywania wartości xd
No ale nie ma to jak ktoś z większym doświadczeniem spojrzy na kod.

P.S - mam nadzieję, że posprzątam kod, ale na wszelki wypadek prosze moderacje o nie zamykanie, to zrobię kolejny commit do wglądu.

0
BartoSAS napisał(a):

Wielkie dzięki - lektura już dawno zakupiona i używana, ale nie zawsze zdawałem sobie sprawę, że jakiś punkt mnie dotyczy :p

U mnie ta książka leży przy łóżku albo na biurku. :) Razem z kolejną -- "42 sposobami". :)

I prawie każdy punkt mnie dotyczy... :)

0

Kolego, wrzucasz string przez wartosc jako argumenty do funkcji - uzywaj referencji bo to straszny blad. Mowie o lini:

void Person::newContatc(const std::string v_name, const std::string v_phonen, const std::string v_address, const std::string f_name, const std::string l_name)

powinno byc

void Person::newContatc(const std::string &v_name, const std::string &v_phonen, const std::string &v_address, const std::string &f_name, const std::string &l_name)

2

No i powiedz mi tutaj kto ci zwolni pamiec tutaj:
Person *temp_p = new Person;
?
rob to conajmniej z std::shared_ptr, cos jak std::make_shared<Person>(), Person powinien miec normalny konstruktor, a nie dodatanie elementow jeden po drugim. Po drugie nie musisz pisac swojego konstruktora kopiujacego, kompilator zrobi to za Ciebie.

1
void Person::newContatc(const std::string &v_name, const std::string &v_phonen, const std::string &v_address, const std::string &f_name, const std::string &l_name)

W tej metodzie jest w ogóle za dużo argumentów. Biorąc pod uwagę, że pierwsze trzy argumenty służą do inicjalizacji pól należących do klasy Contact,
można bezpośrednio użyć obiektu tej klasy w liście argumentów

void Person::newContatc(const Contact& contact, const std::string &f_name, const std::string &l_name)

ograniczając jej długość. Lub nawet

void Person::newContatc(const Contact& contact, const Person& person)

i wywołać ją

test.newContatc{{"Test","Test","Test"},{"Test","Test"}};
2

Skoro Person dziedziczy po Contact to sygnatura powinna wyglądać tak:
void Person::NewContact( Contact* contact );

A ogółem jak dla mnie to cały kod nie ma sensu.

  • Metody klasy Contact są nieintuicyjne. Nazwa klasy jest w liczbie pojedynczej tymczasem mamy do dyspozyji metody head, tail i sort_by.
  • Klasa Person i Company dziedziczy po Contact, ale Person ma statyczną tablice z instancjami Person. Czyli nie można do niej dodać innej klasy niż Person. Po co więc to dziedziczenie?
0
tajny_agent napisał(a):

A ogółem jak dla mnie to cały kod nie ma sensu.

  • Metody klasy Contact są nieintuicyjne. Nazwa klasy jest w liczbie pojedynczej tymczasem mamy do dyspozyji metody head, tail i sort_by.
  • Klasa Person i Company dziedziczy po Contact, ale Person ma statyczną tablice z instancjami Person. Czyli nie można do niej dodać innej klasy niż Person. Po co więc to dziedziczenie?

Zgadza się, ta tablica tam trafiła przez pomroczność i chwilowe zaślepienie umysłowe.
Po przemyśleniu głębszym, żeby nadać sens dla tego kodu uznałem, że może wyjść z tego pseudo "Książka telefoniczna".

Wpadłem na koncepcję rozwiązania tego poprzez vector wskaźników klasy bazowej, tylko teraz zastanawiam się, czy właśnie metody 'head', 'tail' mają sens bytu, jeżeli żeby ich użyć muszę najpierw stworzyć kontener, gdzie będę mógł przeprowadzać takie zabiegi. Ogólnie czuję tutaj po kościach, że źle rozplanowałem te klasy, ale uważam że to jeszcze nic straconego.

Starałem się poprawić wszystkie wniesione powyżej sugestie i mam nadzieję, że kod będzie czytelniejszy i lepszy, aczkolwiek po kasacji ze względu na głupi pomysł statycznej tablicy zrobiłem z ilością kodu "krok wstecz", by podpytać się o sens istnienia tych 3 metod w klasie bazowej.

1

Contacts(int arg1,std::string arg2,std::string arg3,std::string arg4):
index{arg1}, name{arg2}, phone_number{arg3}, address{arg4} {}
...

Person(int arg1,std::string arg2,std::string arg3,std::string arg4,std::string arg5, std::string arg6):
Contacts{arg1, arg2, arg3, arg4}, first_name{arg5}, last_name{arg6}
...


Jeśli już przekazujesz argumenty przez wartość, zrób tak:
```cpp
Contacts(int arg1,std::string arg2,std::string arg3,std::string arg4):
	index{arg1}, 
	name{std::move(arg2)}, 
	phone_number{std::move(arg3)}, 
	address{std::move(arg4)} 
{}
...

Person(int arg1,std::string arg2,std::string arg3,std::string arg4,std::string arg5, std::string arg6):
	Contacts{arg1, 
		std::move(arg2), 
		std::move(arg3), 
		std::move(arg4)}, 
	first_name{std::move(arg5)}, 
	last_name{std::move(arg6)}
...

pozbędziesz się bezsensownych kopiowań

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