Datei einlesen und in Structvektor schreiben

[Mr Joe]

Hallo an alle,

ich versuche mich gerade an einem Programm, mit dem ich historische Kursdaten aus .csv Dateien einlese, um damit noch weitere Sachen zu machen (die erstmal unwichtig sind).

Ich wollte einfach mal wissen, was ihr davon haltet, was ich bisher geschrieben habe. Das ist sicherlich keine ideale Lösung, aber hauptsache es funktioniert.

main.cpp:

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include "Day.h"

std::ifstream file;    //Die Datei
std::vector<Day> chartdata;

//Open File
void openfile()
{
    file.open("table.csv");		//file.csv wird geöffnet
    if(!file.is_open())			//Fehlermeldung   
        std::cerr << "Fehler beim Öffnen!" << std::endl;

	size_t    pos;
    std::string search;
    search = "Adj Close";		//Letzte Spaltenüberschrift, um an den Anfang der 2. Zeile zu gelangen
    std::string line;			//Zum Überspringen der Überschriften
    //Überspringe die Überschriften und setze den Cursor auf das erste Datum
	getline(file, line);        //1. Zeile in line schreiben
    pos = line.find(search);    //suche den String search
    file.seekg(pos + search.length() + 1);			//Setze Cursor hinter die Überschrift

}

std::istream& readdata(Day& day)
{
    getline(file, day.date, ',');
    getline(file, day.open, ',');
    getline(file, day.high, ',');
    getline(file, day.low, ',');
    getline(file, day.close, ',');
    getline(file, day.volume, ',');
    getline(file, day.adjclose, '\n');
    return file;
}

//Read File
void readfile()
{
	chartdata.resize(1);
	for(int i = 0; file.good();i++)
	{
		Day day;
		readdata(day);
		if(i == 0)
		{
			chartdata[0].date		= day.date;
			chartdata[0].open		= day.open;
			chartdata[0].high		= day.high;
			chartdata[0].low		= day.low;
			chartdata[0].close		= day.close;
			chartdata[0].volume		= day.volume;
			chartdata[0].adjclose	= day.adjclose;
		}

		else
			chartdata.push_back(day);
        }
    return;
}

//Close File
void closefile()
{
    file.close();        //file wird geschlossen
}

int main()
{
	openfile();   
	readfile();
	closefile();
	getchar();
}

Day.h:

#include <iostream>

//Struct für jeden Tag
struct Day
{
    //Kursdaten
    std::string        date;
    std::string        open;
    std::string        high;
    std::string        low;
    std::string        close;
    std::string        volume;
    std::string        adjclose;
};

Blöd finde ich, wie ich in die 2. Zeile gelange (nach dem Öffnen der File), aber es funktioniert.
Ist der Code hart dämlich oder akzeptabel?
Es sollte vielleicht erwähnt werden, dass ich mehrere Jahre Historie benötige. Das heißt, es werden ganz schnell mal über 1000 Days, oder gar mehrere 1000e.

Bin auf Kritik gespannt

[out]

Fast akzeptabel
- Keine globale Variablen verwenden
- openfile und closefile wegmachen
- readdata durch istream>> Überladung ersetzen
- file.good() durch file>>day; ersetzen
- Wieso ist da so eine Leere in deiner Klasse zwischen string und Bezeichner? Da bekommt man ja schon fast Angst.
- CSV mit Komma? Ich nehm da immer Strichpunkt.

[Mr Joe]

Hallo out,

danke erstmal für deine Antwort

Ich habe jetzt openfile und closefile in die main kopiert. Außerdem gibt es keine globalen Variablen mehr.
Das Komma als Trennzeichen wird so geliefert. Mich hat es ehrlich gesagt auch gewundert und hatte da sogar ursprünglich Semikolons stehen
Sogar die große Lücke in der Day.h ist weg

Aber das mit den ">>" musst du mir erklären Leider ist Google (oder ich) unfähig danach zu suchen.
"readdata durch istream>> Überladung ersetzen" verstehe ich leider gar nicht.
Und "file.good() durch file>>day; ersetzen"? day wird doch erst in der for-Schleife deklariert und ist an der Stelle noch nicht bekannt. Oder sehe ich das falsch?

[hustbaer]

Ich will hier nur ein paar Kleinigkeiten anmerken. Was aber nicht heisst dass der Rest OK ist, ich hab nur keine Zeit alle Probleme hier zu beschreiben (und einige andere wurden ja auch schon genannt).

Mr Joe schrieb:

//Read File
void readfile()
{
	chartdata.resize(1);
	for(int i = 0; file.good();i++)
	{
		Day day;
		readdata(day);
		if(i == 0)
		{
			chartdata[0].date		= day.date;
			chartdata[0].open		= day.open;
			chartdata[0].high		= day.high;
			chartdata[0].low		= day.low;
			chartdata[0].close		= day.close;
			chartdata[0].volume		= day.volume;
			chartdata[0].adjclose	= day.adjclose;
		}

		else
			chartdata.push_back(day);
        }
    return;
}

Wieso nicht einfach

void readfile()
{
	chartdata.clear();

	while (true)
	{
		Day day;
		readdata(day);
		if (file)
			chartdata.push_back(day);
		else
			break;
	}
}

Und

void openfile()
{
    file.open("table.csv");     //file.csv wird geöffnet
    if(!file.is_open())         //Fehlermeldung   
        std::cerr << "Fehler beim Öffnen!" << std::endl;

    size_t    pos;
    std::string search;
    search = "Adj Close";       //Letzte Spaltenüberschrift, um an den Anfang der 2. Zeile zu gelangen
    std::string line;           //Zum Überspringen der Überschriften
    //Überspringe die Überschriften und setze den Cursor auf das erste Datum
    getline(file, line);        //1. Zeile in line schreiben
    pos = line.find(search);    //suche den String search
    file.seekg(pos + search.length() + 1);          //Setze Cursor hinter die Überschrift

}

finde ich anstrengend zu lesen.

Viel besser so:

void openfile()
{
    file.open("table.csv");
    if (!file)
    {
        std::cerr << "Fehler beim Öffnen!" << std::endl;
        // TODO: Error handling - just printing a message and then continuing as if nothing had happened is NOT the right thing to do.
    }

    // Read first line

    std::string firstLine;
    getline(file, firstLine);

    // Seek to after the column headers.

    std::string const lastColumnName = "Adj Close";
    size_t const lastColumnNameStart = firstLine.find(lastColumnName);
    // TODO: Error handling - lastColumnNameStart might be npos (lastColumnName not found).
    file.seekg(lastColumnNameStart + lastColumnName.length() + 1); 
}

* Kommentare über offensichtliche Dinge entfernt
* Variablen haben ordentliche Namen bekommen
* Kommentare über Dinge entfernt die mit den neuen, besseren Variablennamen nun offensichtlich sind
* Variablen erst dort definieren wo man sie braucht
* Dadurch können wir Variablen oft "const" machen was hilfreich beim Lesen des Codes ist (man sieht sofort "das Ding wird in der Funktion nimmer geändert")

Sonstinge Dinge:
Die Funktion macht kein korrektes Error-Handling. Was natürlich ganz schlecht ist.
Und ich verstehe den Sinn der ganzen "letzten Spaltennamen suchen" Sache nicht.
Normalerweise sind die Spaltennamen in einem CSV File in der ersten Zeile, und NUR diese in der ersten Zeile.
=> Du kannst alles was nach getline(file, firstLine); kommt weg lassen -- das getline macht ja eh schon genau das was du willst, nämlich die ersten Zeile zu "überlesen".

[Mr Joe]

Hey hustbaer,

danke für die ausführliche Antwort!
Du hast mit der Suche der ersten Zeile völlig recht. Urspünglich dachte ich, die Datei würde aus einer einzigen Zeile bestehen. Als ich gesehen habe, dass das nicht so ist, habe ich meinen Code nicht verbessert, sondern weiter bearbeitet und gar nicht mehr darüber nachgedacht

Es ist auch das erste Mal, dass ich einen Vector benutze. Ich wusste gar nicht, dass man mittels

chartdata.push_back(day);

gleich alle Daten rausholt. Danke

Und was du da so unleserlich fandest, damit hattest du völlig recht, fand ich auch.

Bzgl. des Errorhandlings - Wie weit soll ich da gehen? Ich habe nun:

file.open("table.csv");
	if (!file)
	{
		std::cerr << "Fehler beim \x99 \bffnen!\nAnderen Dateinamen eingeben (mit Endung): " << std::endl;
		// TODO: Error handling - just printing a message and then continuing as if nothing had happened is NOT the right thing to do.
		std::string newfile;
		std::cin >> newfile;
		file.open(newfile);
		if(!file)
		{
			std::cerr << "Das hat auch nicht geklappt! Vielleicht keine Datei vorhanden?" << std::endl;
			std::abort();
		}
	}

Somit wird das Programm nicht aus Versehen weiter ausgeführt. Ich habe überlegt, ob ich noch nach dem Dateinamen fragen soll, bzw. nach einer neuen Datei zum Öffnen. Da ich aber das ganze möglichst einfach haben will, möchte ich möglichst einfach wenn es fertig ist eine .exe ausführen, die mit möglichst wenig Input auskommt.