Verbesserungsvorschläge und Kritik

[_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.

[n0stradamus]

Hi,

habe das Problem mit einer Queue gelöst, hat der Übersichtlichkeit halber eine Funktion mehr gefordert aber jetzt läuft das Programm schön und stabil
Vielen Dank für Eure Hilfe, irgendwann bin ich ja vllt auch so weit, dass ich euch mal helfen kann :p

Eines hat mich allerdings doch gestört:

Angenommen, vorher wurde definiert:

string *real;
real = new string;

//Über den Sinn lässt sich jetzt streiten

Dann produziert diese Zeile einen Fehler, wenn ich den Pointer durch einen richtigen String ersetze allerdings nicht:

if((*real).find(' ')==0)
	{
		for(int o=0;0<(*real).size();o++)
			{(*real)[o] = (*real)[o+1];} //Leerstelle an erster Stelle eliminieren, wenn es eins gibt
	}
//Zum besseren Verständnis:
//real ist der String, der die fertig bearbeitete Beschreibung der Person enthält

Warum ist das so?
delete habe ich hinterher natürlich benutzt

[Nexus]

Da du jetzt keinen Zeiger mehr hast, musst du auch nichts dereferenzieren, bevor du auf Memberfunktionen zugreifen willst.

// Zeiger auf dynamisch angelegtes Objekt
std::string* ptr = new std::string("hallo");
(*ptr).clear();  // Zugriff über (*ptr).
ptr->clear();    // oder über ptr->
delete ptr;

// Automatisches Objekt
std::string obj("hallo");
obj.clear();     // Zugriff über obj.

Bei automatischen Objekten hast du hoffentlich kein delete benutzt.

Ausserdem ist der folgende Code ziemlich ineffizient.

if((*real).find(' ')==0)
	{
		for(int o=0;0<(*real).size();o++)
			{(*real)[o] = (*real)[o+1];} //Leerstelle an erster Stelle eliminieren, wenn es eins gibt
	}

Wenn du wissen willst, ob an erster Stelle ein Leerzeichen ist, schaust du doch dort nach. Was du machst, ist aber den ganzen String nach Leerzeichen durchsuchen und überprüfen, ob das erste Vorkommen an der Position 0 steht. Fürs Löschen gibt es auch bereits vorhandene Funktionalität. Schau zuerst hier, ob du nicht etwas nochmal von neu erfindest, std::string hat sehr viele Methoden (etwas zu viele...) und ausserdem gibts noch die generischen STL-Algorithmen.

Eine schnellere und einfachere Implementierung sähe so aus:

if (real[0] == ' ')
    real.erase(0);

[n0stradamus]

Nexus schrieb:

Da du jetzt keinen Zeiger mehr hast, musst du auch nichts dereferenzieren, bevor du auf Memberfunktionen zugreifen willst.

// Zeiger auf dynamisch angelegtes Objekt
std::string* ptr = new std::string("hallo");
(*ptr).clear();  // Zugriff über (*ptr).
ptr->clear();    // oder über ptr->
delete ptr;

// Automatisches Objekt
std::string obj("hallo");
obj.clear();     // Zugriff über obj.

Bei automatischen Objekten hast du hoffentlich kein delete benutzt.

Dereferenziert und -d habe ich bei dem automatischen Objekt nicht^^
Dann muss ich meine Frage nochmal anders formulieren:
So wie du den Code oben siehst, als dereferezierte Version mit der geltenden Annahme, dass real ein Zeiger auf einen dynamisch angelegten String ist, so produziert das einen Fehler.

Hebe ich allerdings die Dereferenzierung auf und mache

real

zu einem automatischen Objekt, so läuft das Ganze fehlerfrei. Warum?
Einen Unterscheid beim Verbrauch von Speicher gibt es doch nicht oder ?

real wird beim Schließen der Funktion in der es sich befindet ja wieder gelöscht.

PS:
Aber Danke für den Hinweis, dass es meine Funktion schon in anderer Form gibt^^
Wenn es so viele Extrabefehle gibt, muss man ja gar nicht mehr basteln

[Nexus]

n0stradamus schrieb:

So wie du den Code oben siehst, als dereferezierte Version mit der geltenden Annahme, dass real ein Zeiger auf einen dynamisch angelegten String ist, so produziert das einen Fehler.

Was für einen Fehler? Kannst du den momentanen Code nochmals posten?

[n0stradamus]

Klaro,

Das hier läuft schief:

if((*real).find(' ')==0)
    {
        for(int o=0;0<(*real).size();o++)
            {(*real)[o] = (*real)[o+1];} //Leerstelle an erster Stelle eliminieren, wenn es eins gibt
    }
//Zum besseren Verständnis:
//real ist der String, der die fertig bearbeitete Beschreibung der Person enthält

Diese Variante hingegen läuft problemlos:

if(real.find(' ')==0)
	{
		for (int l=0;l<real.size();l++)
		{
			real[l]=real[l+1];
		}
		//Leerstelle an erster Stelle eliminieren, wenn es eine gibt
	}

Der restliche Funktionsaufbau ist genau gleich (mal von der Initialisierung von real abgesehen)

[Nexus]

Naja, ich habe eigentlich nach dem Fehler gefragt, und die Deklaration wäre auch nicht schlecht gewesen.

Wie auch immer, der Funktionsaufbau ist eben nicht gleich.

for(int o=0;0<(*real).size();o++)
//          ^

Der Code wäre übrigens lesbarer, wenn du mehr Leerzeichen hättest und operator-> verwenden würdest.