c++ odzczyt z pliku

[piotrekkkkkk]

Cześć

 

 

Mam program, który pobiera z pliku dane i przetwarza je, ale jest problem (tak mi się wydaje) z odczytem. Program przetwarza tylko 1 i dwie ostatnie linijki dokumentu.

Tak wygląda funkcja odczytująca

 

vector<Database> readData(const string name) {
    
    vector<Database> workers;
    
    ifstream file(name.c_str(), ios::in);
    
    if(!file.good()) {
        cout<<"Blad podczas otwierania pliku\n";
        exit;
    }
    else {
        string temp;
        int lineCount = 0;
        
        while(!file.eof()) {
            lineCount++;
        
            Database tempWorkers;
        
            string name, surname;
            short age, hours;
            float rate;
        
            for(int i=0;i<lineCount;i++) {
                
                 file>>name;
                 file>>surname;
                 file>>age;
                 file>>rate;
                 file>>hours;
                
                 tempWorkers.name = name;
                 tempWorkers.surname = surname;
                 tempWorkers.age = age;
                 tempWorkers.rate = rate;
                 tempWorkers.hours = hours;
            }
            workers.push_back(tempWorkers);
        }
        return workers;
    }
}

A tak wyswietlanie przetworzonych danych

 

void showData(vector<Database> &workers) {
    
    
    int j;
    
    for(int i=0;i<workers.size();i++) {
        
        j=i+1;
        Database tempWorkers = workers.at(i);
        
        cout<<"\nPracownik nr "<<j
            <<"\nImie i nazwisko: "<<tempWorkers.name<<" "<<tempWorkers.surname
            <<"\nWiek: "<<tempWorkers.age
            <<"\nStawka: "<<tempWorkers.rate<<"PLN"
            <<"\nIlosc przepracowanych godzin: "<<tempWorkers.hours<<"H"
            <<"\nWynagrodzenie: "<<calculateSalary(&tempWorkers)<<"PLN\n";
    }
}

[Sopelek997]

1. po co ci wiedzieć ile jest linii

2. po co ci ta pętla wewnątrz

3. od takich rzeczy jak robisz tu

if(!file.good()) {
        cout<<"Blad podczas otwierania pliku\n";
        exit;
    }

są wyjątki

 

4. ta zmienna 'j' tylko zaciemnia kod

5. flagę eof powinno się sprawdzać po próbie odczytania danych, nie po

6. po co przekazuje string przez wartosc jako const?

7. dlaczego klasę nazwałeś Database, jeśli jedynie co robi to przechowuje JEDEN wpis.

8. po co te zmienne tymczasowe.

9. jeśli wyświetlasz dane to ich raczej nie modyfikujesz, więc dobrze jest parametr przyjmować jako const

10. uzywaj preinkrementacji jeśli postinkrementacja nie jest konieczna

11. '.at' używaj kiedy nie jesteś pewien czy element jest w zakresie kontenera, bo w przeciwnym wypadku nie potrzebnie jest to sprawdzane. W std::vector domyślnie używa się operatora []

12. możesz korzystać z move semantics przy wstawianiu wpisów do vectora, zaoszczędzisz na niepotrzebnym kopiowaniu stringa w klasie Database.

workers.push_back(std::move(tempWorkers));

albo i lepiej

workers.emplace_back(tempWorkers);

bo nie będzie najpierw domyślnie tworzyć obiektu

[Pancake]

Moje 3 grosze:

- Warto wyrobic sobie nawyk pisania std::vector, std::string zamiast using namespace.

- Unikaj jak ognia przekazywania przez wartosc. Albo referencja (std::string&) albo const referencja (const std::string&). Wtedy unikasz kopiowania, a const gwarantuje, ze twoja funkcja nie  naruszy przekazywanego obiektu. Obecnie, robisz kopie i niepotrzebnie oznaczasz ja jako const (bo nie dzialasz na orginalnym stringu ktory przekazal ci caller).

- Czy nie dostales ostrzezenia o deklaracji "string name" w ktorejs tam linijce, przeciez to koliduje z nazwa argumentu wywolania funkcji.

- "Database" brzmi raczej jak caly kontener na wszystkie rekordy, a nie pojedyncza osobe.

- std::ifstream ma przeladowany konstruktor z std::string, nie musisz wyciagac const char* metoda c_str()

- Wystarczy if(!file) // oblsuga bledu

- Nie uzywaj exit w przyapdku bledu! Rzuc wyjatkiem. Generalnie od funkcji oczekuejsz jakiejs funkcjonalnosci, zeby zrobila to czy tamto, a w razie niepowodzenia - ma zglosic wyjatek ktory obsluzy caller, a nie ubijac caly proces

- Jak wynika z twojego kodu, Database to pojedynczy rekord, dlatego nazwa zmiennej tempWorkers (liczba mnoga) jest troszke mylaca.

- Za duzo zmiennych loklanych. Mozesz bezposrednio wczytywac do membera (file>>tempWorker.name;). Widzisz w forze to paskudne kopiowanie - moglbys w ten sposob tego uniknac.

Druga czesc:

- Zmienna j jest naprawde niepotrzebna. Mozesz po prostu pisac i+1

- std::vector ma milusnski operator[], znacznie ladniejsze i czytelniejsze od at()

- Moznaby bylo uzyc range fora lub iteratora do przelatywania tablicy, ale to kwestia sporna (teoretycznie wywolujesz size() co kazdy obrot petli, a przeciez rozmiar sie nie zmienia)

- Moja uwage przykula funkcja calculateSalary(&tempWorkers). Gdyby ta funkcja przyjmowala pracownika przez referencje, nie musialbys pisac & a do tego nie ryzykowalbys uzycia wskaznika (bo z tego co widze, calculateSalary przyjmuje pracownika* jako argument). Najoptymalniej byloby przyjac go jako const Pracownik& - dlaczego? Poniewaz funkcja tylko liczy cos na podstawie memberow, wiec const gwarantuje ze kalkualtor nic nie zmodyfikuje - consty sa BAAARDZO WAZNE!! Do tego & po to, aby nie kopiowac ani nie ryzykowac uzycia raw pointera.

 

 

qq zapomnialem o ogonkach mam nadzieje ze sie rozczytasz