Verbesserungsvorschläge und Kritik

[n0stradamus]

Hallo,

ich hoffe ich bin hier im richtigen Unterforum.
Wie der Titel schon sagt, meine Anwendung läuft nicht auf anderen Rechnern.
Beides 32-bit Systeme, ich vermute mal dass es daran liegt.
Erstellt wurde das ganze mit MS Visual C++ 2010 Express Beta 2 (Die 2008er Version habe ich auch probiert, lief auf dasselbe hinaus - Anwendung flackert nur kurz auf und schließt sich wieder, die Tips im FAQ habe ich ausprobiert - ohne Erfolg)
Code::Blocks brachte Abhilfe, das Programm ist jetzt lauffähig auf anderen PCs *freu*

Nachdem ich mich durch André Willms 'C++ Programmierung lernen' durchgearbeitet habe, habe ich folgendes Programm entworfen:

#include <iostream>
#include <fstream>
#include <conio.h>

using namespace std;

void creawrite(char* filename, char* name, int* line, char* secpfad)
{
	int i;
	char p[500];
	char kar[100];
	char* k=kar;
	strcpy(k,secpfad);
	strncat(k,filename,100);
	ifstream main(name);
	ofstream person(k);
	for (unsigned int i=0;i<(*line);i++)
	{
		main.getline(p,500);
	}

	for (i=0; p[i] != *":"; i++) {} //Abstand bis zum Doppelpunkt messen
	i++; 							// Doppelpunkt überspringen, soll ja nicht im Enddokument stehen
	if (p[i]==*" ")
	{
		i++;			//etwaiges Leerzeichen eliminieren
	}
	for (unsigned int z=0;z<strlen(p);z++, i++)
	{
		p[z]=p[i];		//Stringcontent nach vorne 'holen'
	}
	person << p << endl;
	cout << "Datei <" << k << "> wurde geschrieben";

	person.close();
	main.close();
	//aufräumen =)
	cout << endl << "Zeile " << *line << " fertig!" << endl;

}

char* readname(char* pfad, char* name,int* line)
{
	unsigned int i=0;
	static int count=0;
	ifstream main(pfad);
	(*line)++;
	for (unsigned int i=0;i<(*line) && (!main.eof()); i++)
	{
		main.getline(name, 50);
	}
	char* p = new char[50];
	cout << endl << endl << "Zeile einlesen...";
	for (i=0; name[i] != *":";i++)
	{
		p[i]=name[i];
	}
	cout << endl << "Einlesen fertig...";
	main.close();
	p[i]   = *".";
	p[i+1] = *"t";
	p[i+2] = *"x";
	p[i+3] = *"t";
	p[i+4] = *"\0";
	return p;
}

int main(){

	int line=0, lines;

	char pfad[100], name[50], secpfad[100];
	cout << "Abikommentarreader/-writer v 0.3" << endl
		 << "-----------------------------------------------------------------------" << endl << endl;

		cout << "Pfad zu der zu bearbeitenden Datei (.txt): ";
		cin.getline(pfad, 100);
		ifstream main(pfad);

		while (!main)
		{
			cout << "!!! Die Datei konnte nicht geoeffnet werden !!!" << endl;
			cout << "Pfad zu der zu bearbeitenden Datei (.txt): ";
			cin.getline(pfad, 100);
			ifstream main(pfad);
		}

	cout << endl << "Speicherort der erstellten Dateien (Backslash nicht vergessen): ";
	cin.getline(secpfad,100);
	if(secpfad[strlen(secpfad)] != *"\ ")
	{
		strncat(secpfad,"\\0 ", 1);
	} //Pfad vervollständigen, falls noch nicht getan

	char* p = new char [501];
	for (lines=-1; !main.eof();lines++)
	{
		main.getline(p,500);
		}		//Zeilen zählen, hier muss was besseres her
	cout << endl << "Zeilen: " << lines;
	delete [] p;
	for (unsigned int x=0;x <= lines;x++)
	{
		creawrite(readname(pfad, name, &line), pfad, &line, secpfad);
		 //Dateiname und Speicherpfad zusammenfügen         ^ 
                //dateiname bestimmen und datei erstellen, beschreiben,schließen

	main.close(); //zu
	getch();
	return 0;
}

Das Programm soll Mitarbeitern des Abizeitungsteams helfen, die Datenmenge schneller zu verarbeiten.
Jedes Stufenmitglied schickt eine txt-Datei mit folgender Formatierung an den Verwalter:
[...]
Name: Beschreibung
Name: Beschreibung
[...]

Das Programm öffnet nun eine Datei mit dem Namen des Schülers und schreibt das hinein, was hinter dem Doppelpunkt nach dem Namen stand - so lange bis die Datei eben zu Ende ist.

Eine Frage zum Code habe ich aber:

in creawrite():
der Funktion wird ja ein pointer zu einem mit new erstellten Feld übergeben.
Wenn ich der Funktion jetzt sage, sie soll nachdem der Pointer die Notwendigkeit verloren hat, den Pointer deleten [],
wird darauf leider nichts - Fehlermeldung...weshalb?

Über Optimierungen und andere Vorschläge würde ich mich freuen

[Nexus]

Was mir gerade auffällt:

• Der Code erinnert viel eher an C als an C++. Die ganzen str...() -Funktionen, die char -Arrays und -Zeiger, die manuelle Speicherverwaltung. In C++ hast du die STL, std::string und Konsorten, welche dir viele Arbeit und Mühe abnehmen. Vielleicht hilft dir das als Einstieg.
• Du hast Memory Leaks. Der Speicher, den die Funktion readname() für p belegt, wird nirgends freigegeben.
• Magic Numbers im Code sind schlecht, weil sie die Wartbarkeit erschweren und Fehler fördern. Nimm eine aussagekräftige Konstante statt 500, oder – noch besser – lege dich nicht auf eine fixe Grösse fest (siehe dafür Punkt 1), weil dich das in der Flexibilität einschränkt und langfristig nur Probleme bringt (was ist, wenn plötzlich 502 Zeichen benötigt werden? Oder nur drei? Sinnlos Speicher verschwenden?).
• *"x" – sowas hab ich noch nie gesehen. Nimm für char -Literale einfache Anführungszeichen, also 'x' .
• main ist ein schlechter Bezeichner.

[n0stradamus]

Danke für die Antwort, werde mir das mit deinen Link mal anschauen
Die Problematik des Memory Leaks war mir bewusst, aber jedesmal wenn ich
delete [] p in creawrite()
eingebaut habe, gab es nach einem Durchgang (Zeile) jede Menge Fehlermeldungen...
Was mich bisher vor der Verwendung von strings abgehalten hat, war die Tatsache dass ich nicht wusste, wie groß ein String ist. Wächst und schrumpft er dynamisch mit seinem Inhalt, mal etwas doof gefragt ?

Andere Funktionen als die str...() kenne ich nicht. Gibt es denn noch andere Möglichkeiten die Länge eines Strings zu bestimmen. Zeiger finde ich dafür ganz praktisch, weil man schon das ein oder andere Byte, als auch Zeit damit spart.
Aneinanderhängen ist ja bei Strings mit += möglich.

Vielen Dank soweit,

n0stradamus

[SeppJ]

std::string ist vollautomatisch in jeder Hinsicht, das ist ja gerade das praktische daran.

[n0stradamus]

SeppJ schrieb:

std::string ist vollautomatisch in jeder Hinsicht, das ist ja gerade das praktische daran.

Ist ja genial, warum wird das dann mit keinem Wort in dem Buch erwähnt ?...
Aber deswegen bin ich ja im Forum, danke für den Hinweis^^

Woran liegt es eigentlich, dass mit MS Visual Studio Express 2008/2010 Beta 2 erstellte Programme nur auf meinem Rechner laufen?
Sobald ich Code::Blocks verwende, ist das kein Problem mehr

[asc]

n0stradamus schrieb:

Die Problematik des Memory Leaks war mir bewusst, aber jedesmal wenn ich delete [] p in creawrite() eingebaut habe, gab es nach einem Durchgang (Zeile) jede Menge Fehlermeldungen...

Grundsätzlich:
a) Zu jedem new genau ein delete, zu jedem new[] genau ein delete[]
b) Man sollte new/delete möglichst innerhalb der gleichen Funktion verwenden, nicht aber dies aufteilen (dann wird es sehr unintuitiv mit der Bearbeitung - Muss ich nun den Speicher freigeben/reservieren oder nicht?).

n0stradamus schrieb:

Was mich bisher vor der Verwendung von strings abgehalten hat, war die Tatsache dass ich nicht wusste, wie groß ein String ist. Wächst und schrumpft er dynamisch mit seinem Inhalt, mal etwas doof gefragt ?

std::string verwaltet seinen Speicher selbst und ist dynamisch. Die Stringklasse bietet diverse Methoden an (u.a. auch um die Länge zu bestimmen).

n0stradamus schrieb:

Gibt es denn noch andere Möglichkeiten die Länge eines Strings zu bestimmen. Zeiger finde ich dafür ganz praktisch, weil man schon das ein oder andere Byte, als auch Zeit damit spart.

Mache dich mit der string-Klasse und deren Methoden vertraut. Zudem sehe ich es im ersten Schritt wichtiger an, das ein Programm möglichst fehlerfrei läuft, und erst im Zweiten sollte man - sofern man sicher weiß wo die Performance verloren geht (Was nur ein Profiler sagen kann, das Bauchgefühl liegt in der Regel in 90% daneben) - dann optimieren.

n0stradamus schrieb:

Aneinanderhängen ist ja bei Strings mit += möglich.

Aber nur bei der string-Klasse, nicht bei C-Strings.

[asc]

n0stradamus schrieb:

Woran liegt es eigentlich, dass mit MS Visual Studio Express 2008/2010 Beta 2 erstellte Programme nur auf meinem Rechner laufen?

1. Als Releaseversion linken.
2. Die Runtime muss entweder statisch in das Programm gelinkt werden, oder auf dem Rechner verfügbar sein. Siehe z.B. dies hier für den 2008er [in der Regel sollte die aber meist durch andere Programme schon vorliegen]. Beim 2010er sieht es sicherlich (da neu) Anders aus.

n0stradamus schrieb:

Sobald ich Code::Blocks verwende, ist das kein Problem mehr

Code::Blocks linkt glaube ich von Haus aus die Runtime statisch zum Programm hinzu.

[_matze]

n0stradamus schrieb:

Die Problematik des Memory Leaks war mir bewusst, aber jedesmal wenn ich
delete [] p in creawrite()
eingebaut habe, gab es nach einem Durchgang (Zeile) jede Menge Fehlermeldungen...

Auch, wenn du wohl lieber std::string verwenden wirst (was auch durchaus ratsam ist):

Das wäre ja auch ein ziemliches Durcheinander . Du hättest es dir also so gedacht?

- Speicher anfordern in readname
- Speicher freigeben in creawrite

Damit hätte erstens der ersten Parameter von creawrite zwingend das Ergebnis des Aufrufs von readname sein müssen (du schränkst die Funktion ziemlich ein). Und zweitens dürftest du die Funktion readname niemals ohne folgende creawrite ausführen. Damit wäre ein wesentlicher Sinn von Funktionen (Unabhängigkeit, Wiederverwendbarkeit) ad absurdum geführt.

Zumindest "richtiger" wäre es, hinter die (getrennten) Aufrufe von readname und creawrite ein delete[] zu packen. Aber sowas ist auch schlecht, wurde ja schon erwähnt. Früher oder später vergisst man einfach mal, dass dem Aufruf von readname immer zwingend ein delete[] folgen muss. Ein Funktionsaufruf sollte immer problemlos möglich und nicht an Bedingungen geknüpft sein.

Besser ist es, selbst genügend Speicher anzufordern, der Funktion readname einen Zeiger darauf zu übergeben, und anschließend nach weiterer Verarbeitung (z.B. durch creawrite) selbst das delete[] auszuführen. So sind new und delete immer auf derselben Ebene. Das schafft Übersicht und schont das Gedächtnis.

[pumuckl]

n0stradamus schrieb:

main.close(); //zu

Hälst du das für einen besonders hilfreichen Kommentar? Oder anders gefragt - verdeutlicht der Kommentar etwas, was man sonst nicht ohne weiteres aus dem Code lesen kann?

Zusätzlich solltest du dir eine einheitlichen Einrückungsstil zulegen. Das sieht bei deinem geposteten Code eher nach Zufall aus, mal ganz davon abgesehen dass die letzte for-Schleife keine schließende Klammer zu haben scheint.

[_matze]

Und noch was zu lesen zum Thema "meine Anwendung läuft bei anderen nicht!"...

http://blog.kalmbach-software.de/de/2008/03/05/wie-man-statisch-gegen-die-c-runtime-crt-linkt/

[n0stradamus]

_matze schrieb:

Und noch was zu lesen zum Thema "meine Anwendung läuft bei anderen nicht!"...

http://blog.kalmbach-software.de/de/2008/03/05/wie-man-statisch-gegen-die-c-runtime-crt-linkt/

Dankeschön

Mittlerweile habe ich die char-Arrays durch strings ersetzt, aber jetzt habe ich ein Problem mit den Filestreams:
ifstream ist ja so definiert, dass es nur char-Parameter nimmt. Den String casten hielt ich für etwas rabiat (hat auch nicht funktioniert^^) und dann war ich schon mit meinem Latein am Ende. Google wollte nichts Passendes ausspucken, könnt ihr mir weiterhelfen, bitte ?

@pumuckl: Der Kommentar ist schon etwas unnötig, aber zu Übungszwecken doch praktisch, so sehe ich es zumindest.

[brotbernd]

string s;
ifstream ifs(file);
if (ifs.good()){
	ifs.seekg (0, std::ios::end);
	size_t length = ifs.tellg();
	ifs.seekg (0, std::ios::beg);

	std::vector<char> buffer(length+1,0);

	ifs.read (&buffer[0],length);
	s = &buffer[0];
}

[SeppJ]

n0stradamus schrieb:

ifstream ist ja so definiert, dass es nur char-Parameter nimmt. Den String casten hielt ich für etwas rabiat (hat auch nicht funktioniert^^) und dann war ich schon mit meinem Latein am Ende.

std::string::c_str(). Siehe auch hier:
http://www.cplusplus.com/reference/string/string/c_str/

Google wollte nichts Passendes ausspucken, könnt ihr mir weiterhelfen, bitte ?

Nun komm, veralbern kann ich mich auch selber:
http://www.google.com/#q=ifstream+string

[n0stradamus]

Danke für die Hilfen

Auch wenn es immernoch nicht funktioniert, ist es doch schick was man da so alles machen kann. Wieviele Funktionen genau denselben Fehler produzieren
Folgendes springt nach dem Zeilenzählen immer hoch:

Debug Assertion Failed!

Program:[Pfad]
File:f:\dd\vctools\crt_bld\self_x86\crt\src\dbgdel.cpp
Line 52

Expression:_BLOCK_TYPE_IS_VALID(pHead->nBlockUse)

For information.....

Mit einem schönen roten Kreuz nebendran^^

Hier nochmal der Quellcode:

#include <iostream>
#include <fstream>
#include <conio.h>
#include <string>

using namespace std;

void creawrite(string* filename, string* name, int* line, string* secpfad)
{
	int i;
	string p,nfilename;
	char *k_cstrptr, *name_cstrptr;

	nfilename= *secpfad;
	nfilename+= *filename;

	k_cstrptr = new char[nfilename.size()+1];
	strcpy(k_cstrptr,nfilename.c_str());
	name_cstrptr = new char[(*name).size()+1];
	strcpy(name_cstrptr, (*name).c_str());

	ifstream hauptdatei(name_cstrptr);
	ofstream person(k_cstrptr);
	for (int i=0;i<(*line);i++)
	{
		std::getline(hauptdatei, p);
	}

	for (i=0; p[i] != ':'; i++) {} //Abstand bis zum Doppelpunkt messen
	i++; 							// Doppelpunkt überspringen, soll ja nicht im Enddokument stehen
	if (p[i]==' ')
	{
		i++;					//etwaiges Leerzeichen eliminieren
	}
	for (unsigned int z=0;z<p.length();z++, i++)
	{
		p[z]=p[i];				//Stringcontent nach vorne 'holen'
	} 
	person << p << endl;
	cout << "Datei <" << *filename << "> wurde geschrieben";
	//aufräumen
	delete[] name_cstrptr;
	delete[] k_cstrptr;

	person.close();
	hauptdatei.close();
	//aufräumen =)
	cout << endl << "Zeile " << *line << " fertig!" << endl;

}

void readname(string* pfad, string* name, int* line)
{
	unsigned int i=0;
	string nameholder;
	char* pfad_cstr;

	pfad_cstr = new char[(*name).size()];
	strcpy(pfad_cstr, (*name).c_str());

	std::ifstream hauptdatei(pfad_cstr);
	(*line)++;

	for (int i=0;i<(*line) && (!hauptdatei.eof()); i++)
	{
		std::getline(hauptdatei, nameholder);
	}
	cout << endl << endl << "Zeile einlesen...";

	for (i=0; (*name)[i] != ':';i++)
	{
		(*name)[i]= nameholder[i];
	}
	cout << endl << "Einlesen fertig...";

	delete[] pfad_cstr;
	hauptdatei.close();
}

int main(){

	int line=0, lines;

	string pfad, name, secpfad;
	cout << "Abikommentarreader/-writer v 0.3" << endl 
		 << "-----------------------------------------------------------------------" << endl << endl;

		cout << "Pfad zu der zu bearbeitenden Datei (.txt): ";
		std::getline(cin,pfad);
		std::ifstream hauptdatei(pfad.c_str());

		while (!hauptdatei)
		{
			cout << endl << "!!! Die Datei konnte nicht geoeffnet werden !!!" << endl;
			cout << "Pfad zu der zu bearbeitenden Datei (.txt): ";
			std::getline(cin, pfad);
			ifstream hauptdatei(pfad.c_str());
		}

	cout << endl << "Speicherort der erstellten Dateien (Backslash nicht vergessen): ";
	std::getline(cin, secpfad);

	string* p = new string;
	for (lines=-1; !hauptdatei.eof();lines++)
	{
		std::getline(hauptdatei, *p);
		}		//Zeilen zählen, hier muss was besseres her
	cout << endl << "Zeilen: " << lines;
	delete [] p;

	for (int x=0;x <= lines;x++)
	{
		readname(&pfad, &name, &line);
		creawrite(&name, &pfad, &line, &secpfad);    
		//dateiname bestimmen und datei erstellen, beschreiben, schließen
	}					

	hauptdatei.close();
	getch();
	return 0;
}

Danke für die Hilfe soweit, das Forum hier ist echt toll!

[Nexus]

Zeile 52 kann keine Assertion geworfen werden, da steht nämlich eine Funktionsdefinition.

Du hantierst immer noch viel zu viel mit manueller Speicherverwaltung und Zeigern, hast die Ratschläge überhaupt berücksichtigt? Falls du das für unnötig hältst, lerne wenigstens mit dem Debugger umzugehen. Das musst du so oder so einmal lernen...

[hustbaer]

n0stradamus schrieb:

@pumuckl: Der Kommentar ist schon etwas unnötig, aber zu Übungszwecken doch praktisch, so sehe ich es zumindest.

Für mich ist das einfach "noise". Total unnötig und verwirrend. Wer nicht weiss was "close()" macht, wird auch vom "zu" Kommentar nix haben.

[_matze]

Nexus schrieb:

Zeile 52 kann keine Assertion geworfen werden, da steht nämlich eine Funktionsdefinition.

Ist ja auch keine Assertion in seiner Datei...

Sieh im Callstack (Aufrufliste) nach, welche Zeile in deinem Code für die Assertion verantwortlich ist. Um da vernünftige Infos zu bekommen, musst du dein Programm natürlich als Debug-Version erstellen.

[n0stradamus]

_matze schrieb:

Nexus schrieb:

Zeile 52 kann keine Assertion geworfen werden, da steht nämlich eine Funktionsdefinition.

Ist ja auch keine Assertion in seiner Datei...

Sieh im Callstack (Aufrufliste) nach, welche Zeile in deinem Code für die Assertion verantwortlich ist. Um da vernünftige Infos zu bekommen, musst du dein Programm natürlich als Debug-Version erstellen.

Also ich habe mal unten auf den Call Stack geschaut, die msvcr100d.dll sei schuld.
Kann ja wohl nicht sein, also muss da irgendwas in meinem Programm falsch sein.
Nach ganz vielen Klicks auf 'Ignore' oder 'Continue' lande ich in 'output.c', Zeile 1643...?!

@Nexus: Mittlerweile habe ich die char-Arrays durch strings ersetzt und brav mit new und delete [] gearbeitet. Was kann ich denn noch tun um den Speicher dynamischer zu verwalten ?

Tut mir echt leid, falls ich hier blöde Fragen stelle, aber so richtig viel Praxiserfahrung haben mir meine beiden Bücher nicht vermittelt

[Nexus]

n0stradamus schrieb:

@Nexus: Mittlerweile habe ich die char-Arrays durch strings ersetzt und brav mit new und delete [] gearbeitet. Was kann ich denn noch tun um den Speicher dynamischer zu verwalten ?

Nexus schrieb:

In C++ hast du die STL, std::string und Konsorten, welche dir viele Arbeit und Mühe abnehmen. Vielleicht hilft dir das als Einstieg.

[n0stradamus]

Hm...dumm von mir, das nicht anzuwenden.
Wie groß ist die Wahrscheinlichkeit, dass beispielsweise ein Vektor mein Programm zum Laufen bringt ?^^

[Nexus]

n0stradamus schrieb:

Wie groß ist die Wahrscheinlichkeit, dass es mein Programm zum Laufen bringt ?^^

Kann ich so nicht sagen, aber ein Blick auf die STL lohnt sich auf alle Fälle. new und delete solltest du jedoch mit der Zeit auch verstehen, auch wenn manuelle Speicherverwaltung im Anwendungscode eher selten vorkommen sollte.