Co można zrobić lepiej

pytanie zadane 21 września 2019 w C i C++ przez JacobNatural Nowicjusz (120 p.)

#include <iostream>
#include <vector>

using namespace std;

class Przyjmowanie_zamowien
{
    string danie;
    int czas_przygotowania;
    int cena;
public:
    Przyjmowanie_zamowien(string nazwa="brak", int czas=0, int koszt=0)
        :danie(nazwa), czas_przygotowania(czas), cena(koszt) {}
    // Przyjmowanie_zamowien & operator=(Przyjmowanie_zamowien &);
    friend ostream & operator << (ostream & ekran, Przyjmowanie_zamowien &p);
    friend  istream & operator >>(istream & ekran, Przyjmowanie_zamowien &p);




};
/*Przyjmowanie_zamowien & Przyjmowanie_zamowien::operator=(Przyjmowanie_zamowien &)
{}*/

class Interfejs
{
    Przyjmowanie_zamowien wsk;
    vector<Przyjmowanie_zamowien> MyVector;
    int a;
public:
    void Dodaj_element();
    void Wyswietl_Tablice();
    Interfejs(): a(5)
    {


        while( a != 0)
        {
            cout <<"Wczytanie danych 1"<<endl;
            cout <<"Wypisanie dancyh 2" <<endl;
            cout << "Zamkniecie aplikacji 0" <<endl;

            if(cin >>a)
            {
                switch(a)
                {
                case 1:
                    Dodaj_element();
                    break;
                case 2:
                    Wyswietl_Tablice();

                    break;
                case 0:
                    cout <<"Zamykamy...";
                    break;
                default:
                    cout <<"Nie znam takiego numeru";
                    break;
                }
            }
            else
            {
                cin.clear();
                while(cin.get() !='\n') {}
                a=5;
                cerr <<"Podales nie poprawne dane" <<endl;
            }

        }
    }




};

void Interfejs::Dodaj_element()
{
    cin >>wsk;
    MyVector.push_back(wsk);
}

void Interfejs::Wyswietl_Tablice()
{
    for(int i=0; i< MyVector.size(); i++ )
    {
        cout << MyVector[i];
    }

}
ostream & operator <<(ostream & ekran, Przyjmowanie_zamowien &p)
{
    ekran << "Nazwa dania " <<p.danie <<" czas przygotowania "
          <<p.czas_przygotowania << " jego cena: " << p.cena<<endl<<endl;
    return ekran;

}

istream & operator >>(istream & ekran, Przyjmowanie_zamowien &p)
{
    int b =1;
    while( b!=0)
    {
        cout <<"Podaj nazwe dania, czas i koszt"<<endl;
        if(ekran >> p.danie>>p.cena>> p.czas_przygotowania)
        {
            return ekran;

        }
        else
        {
            ekran.clear();
            while(ekran.get()!='\n') {}
            cerr <<"Podales niepoprawne dane"<<endl;
            b=1;

        }
    }


}

int main()

{
    Interfejs p;


    return 0;
}

Witam, stworzyłem taką aplikacje w celach edukacyjnych. Chciałbym zasięgnąć opinii bardziej doświadczonych kolegów. Co można zrobić lepiej, co zrobiłem źle.

2 odpowiedzi

odpowiedź 21 września 2019 przez RafalS VIP (122,820 p.)

Tyle przyszło mi na szybko do głowy:

Programować po angielsku
Nie jestem językoznawcą, ale nazwy klas powinne opisywać podmiot, który coś robi. Np nie Przyjmowanie_zamowien tylko PrzyjmowaczZamowien. Nie XmlReading tylko XmlReader.
Niespójne nazewnictwo. Zmienna raz jest CamelCase'em a raz z podkreśleniami.
Formatowanie - niepotrzebne puste linijki. Podwójne spacje. Istnieją formatery do kodu. Np w VS code wciskam ctrl+shift+i i kod jest sformatowany.
Nazwa klasy Interfejs jest za mało znacząca.
Nazwa MyVector nie mówi nic poza tym, że jest to vector, co też jest mało użyteczne bo nawet kiepski IDE podpowie Ci jakiego typu jest zmienna.

Nie korzystasz z pętli typu for each:

for (int i = 0; i < MyVector.size(); i++)

Lepsza wersja:

for (auto&& value : MyVector)

"Przyjmowanie_zamowien wsk;" bezsensowna zmienna składowa. To powinna być zmienna lokalna w metodzie w której jest używana. Nazwa nie dość, że nie opisuje przeznaczenia to jest myląca bo wsk sugeruje jakiś wskaźnik.
int a; mam nadzieje ze nie potrzebuje komentarza.
Cała logika klasy Interfejs w konstruktorze. Tak sie nie robi.
Brak const. Np ostream &operator<<(ostream &ekran, Przyjmowanie_zamowien &p). Przyjmowanie_zamowien powinno być przekazane przez stałą referencje.
Interfejs() : a(5). wtf? Co oznacza a = 5, czemu to wartosc domyslna.

komentarz 21 września 2019 przez tkz Nałogowiec (42,000 p.)

7. dlaczego lepsza zakresowa? Lepiej przedstawia zamiar, ale czy w czym lepsza?

Co do formatowania

On Windows Shift + Alt + F

On Mac Shift + Option + F

On Ubuntu Ctrl + Shift + I

komentarz 21 września 2019 przez RafalS VIP (122,820 p.)

Mniej kodu do napisania. Mniej kodu do analizy - o razu widać, że lecimy po całym zakresie. Nie boję się, że w ciele pętli iterator będzie zmodyfikowany, bo jest to niemożliwe. Mniejsze ryzyko błędu typu one-off.

Co do formatowania, chciałem jedynie zaznaczyć, że coś takiego istnieje. Żeby autor nie wpadł na genialny pomysł poprawiania tego ręcznie. Podałem skrót jako przykład tego jak proste jest sformatowanie kodu w jednym z popularniejszych edytorów bez dodatkowej konfiguracji.

komentarz 21 września 2019 przez tkz Nałogowiec (42,000 p.)

Czy mniej... Na pewno czytelniejszy zamiar. Formatowanie dorzuciłem tak po prostu, żeby autor miał odrazu.

odpowiedź 21 września 2019 przez tkz Nałogowiec (42,000 p.)

Z programowania obiektowego nie jestem mistrzem, więc tu nic nie powiem.

Przestrzeń nazw jest średnim pomysłem.

Polskie nazwy również. Ogólnie stosuje się pisownie camelCase, to już raczej kosmetyczna zmiana.

Nic nie znaczące nazwy.

Podział na pliki, ułatwia rozbudowę dzięki porządkowi.

Vector posiada metodę emplace_back dla obiektów z konstruktorem, jest szybsza bo unikasz kopiowania.

Metody to funkcję, więc dlaczego są pisane wielką literą?

Przy pętlach powinieneś użyć typu size_t, gdy iterujesz po rozmiarze czegokolwiek.

Brak jakiejkolwiek obsługi błędnych danych.

Możliwe, że coś jeszcze, a ja tego po prostu nie widzę.

komentarz 21 września 2019 przez j23 Mędrzec (194,920 p.)

Vector posiada metodę emplace_back dla obiektów z konstruktorem, jest szybsza bo unikasz kopiowania.

Jeśli piszesz o tym fragmencie

cin >> wsk;

MyVector.push_back(wsk);

to użycie emplace_back nic nie zmienia, bo kopia i tak zostanie zrobiona. Za to tak:

MyVector.push_back(std::move(wsk));

robi różnicę.

komentarz 21 września 2019 przez tkz Nałogowiec (42,000 p.)

Nie zmieni nic, bo obiekt jest tworzony wcześniej?

Adds a new element at the end of the vector, after its current last element. The content of val is copied (or moved) to the new element.

Jest potrzebne std::move skoro vektor robi to sam? Chyba, że tutaj domyślnie kopiuje...

komentarz 21 września 2019 przez j23 Mędrzec (194,920 p.)

bo obiekt jest tworzony wcześniej?

Tak. Użycie emplace_back ma sens, gdy chcesz stworzyć obiekt w kontenerze przy użyciu konstruktora parametrycznego (innego niż konstruktor kopiujący/przenoszący).

Jest potrzebne std::move skoro vektor robi to sam?

Samo std::move niczego nie przenosi, jedynie zwraca r-referencję do obiektu, co w tym przypadku powoduje wybranie przez kompilator konstruktora przenoszącego nowo tworzonego obiektu.

komentarz 21 września 2019 przez tkz Nałogowiec (42,000 p.)

Czyli było by trzeba utworzyć obiekty w emplace_back(klasa obiekt)?

Zastosowanie zasady 0/3/5 było by na miejscu? Nie było by trzeba (chyba) wykorzystać std::move.

komentarz 21 września 2019 przez j23 Mędrzec (194,920 p.)

Dam inny przykład:

foo obj1(1, 2, 3, 4, 5);

vec.push_back(obj1); // kopia
vec.push_back(std::move(obj1)); // przeniesienie

foo obj2(1, 2, 3, 4, 5);

vec.emplace_back(obj2); // kopia
vec.emplace_back(std::move(obj2)); // przeniesienie

vec.emplace_back(1, 2, 3, 4, 5); // stworzenie obiektu od razu w vectorze.

Nie było by trzeba (chyba) wykorzystać std::move.

Trzeba by było. Jeśli chcesz przenieść l-vartość, musisz kompilatorowi w sposób jawny to zasygnalizować (domyślnie l-wartości są kopiowane). I od tego jest szablon std::move, który - jak już wspomniałem - zwraca r-referencję do obiektu.

komentarz 21 września 2019 przez tkz Nałogowiec (42,000 p.)

Okey, nie rozumiem ostatniej konstrukcji:

vec.emplace_back(1, 2, 3, 4, 5); // stworzenie obiektu od razu w vectorze.

Jak to ma działać? Co tu jest obiektem?

komentarz 21 września 2019 przez j23 Mędrzec (194,920 p.)

emplace_back przekazuje parametry do odpowiedniego konstruktora, który jest wywoływany w trakcie tworzenia obiektu klasy foo w vectorze. Czyli nie ma kopiowania/przenoszenia obiektu, jak to drzewiej (pre C++11) bywało ;)

Przykładowy kod:

struct foo {
    foo(int a, int b) { std::cout << a << ' ' << b << '\n'; }
    foo(int a, int b, int c) { std::cout << a << ' ' << b << ' ' << c << '\n'; }
};


template<typename T>
struct bar
{
    template<typename... Args>
    void emplace_back(Args&& ...args)
    {
        T(std::forward<Args>(args)...);
    }
};


int main() 
{
    bar<foo> x;
    
    x.emplace_back(1, 2, 3);
    x.emplace_back(4, 5);
    
    return 0;
}