Kleines Progrämmchen, was sagt ihr dazu?

Ich stell euch mal mein erstes, eigenes C++ Programm hier rein und würde
mich über ein bisschen konstruktive Kritik freuen

/*
Programm zur Verschlüsselung von (Text-)Dateien mit Hilfe 
eines symmetrischen Schlüssels. (Entschlüsselung entspricht der
erneuten Verschlüsselung der Zieldatei mit selbem Schlüssel)
Grobe Programmstruktur:

	*Fragt nach Ziel und Quelldatei
	*Initialisiert die entsprechenden Ströme
	*Fragt nach dem Schluessel
	*Initialisiert den Schluessel
	*Führt die Verschlüsselung aus

Auf geht's!
*/

#include <cstdlib>
#include <iostream>
#include <fstream>
#include <string>

#define MAX_PUFFER 50
#define MAX_SCHLUESSEL 50

using namespace std;

//***************Globale Variablen*******************

//Quelle und Ziel
string sQuelle;  // Adresse der Quelldatei
string sZiel;	 // Adresse der Zieldatei

//Der Schluessel
string sSchluessel;
string::iterator itAnfang;   //zeigt auf erstes Zeichen von Schluessel
string::iterator itEnde;	 //zeigt auf letztes Zeichen von Schluessel	
string::iterator itGerade;	 //zeigt auf das gerade zu nutzende Zeichen

//Zwischenpuffer
char cPuffer[MAX_PUFFER];

//Ströme
ifstream ifQuelle;
ofstream ofZiel;

//*****************Funktionen**********************

string holeQuelle(); //erfragt die Adresse der Quelldatei, gibt Adresse dann wieder

string holeZiel();	 //erfragt die Adresse der Zieldatei, gibt Adresse dann wieder

bool initStroeme(string quelle, string ziel);	 //Initialisiert die Datenströme. Gibt bei Erfolg true wieder

string holeSchluessel(); //erfragt den Schluessel, gibt diesen dann wieder.	

bool initSchluessel(string pSchluessel);	//Initialisiert den Schluessel, gibt bei Erfolg true wieder

bool verschluessele();	//Verschluesselt die Dateien, gibt bei Erfolg true wieder

void iteratorErhoehen();//setzt itGerade auf das nächste Zeichen;

int main(int argc, char *argv[])
{
	//Erfrage die Dateien und Initialisiere die Ströme
	if(initStroeme(holeQuelle(),holeZiel()))
		cout << "*Ströme initialisiert!*\n";

	//Erfrage und Initialisiere Schluessel
	if(initSchluessel(holeSchluessel()))
		cout << "*Schluessel erfolgreich initialisiert!*\n";

	//verschluessele die Datei		
	if(verschluessele())
		cout << "*Dateien erfolgreich verschlüsselt!*\n" 
			 << "Der Schlüssel lautete: " << sSchluessel << endl;

    system("PAUSE");
    return EXIT_SUCCESS;
}

string holeQuelle()
{
	string temp;
	cout << "Geben sie die Adresse der Quelldatei ein." << endl;
	cout << "Beispiel: c:\\\\Dateien\\\\quelle.txt" << endl;
	cout << "Ihre Eingabe: ";
	cin >> temp;
	cout << endl;
	return temp;
}

string holeZiel()
{
	string temp;
	cout << "Geben sie die Adresse der Zieldatei ein." << endl;
	cout << "Beispiel: c:\\\\Dateien\\\\ziel.txt" << endl;
	cout << "Ihre Eingabe: ";
	cin >> temp;
	cout << endl;
	return temp;
}

bool initStroeme(string quelle, string ziel)
{
	//initialisiere Lesestrom
	ifQuelle.open((quelle.c_str()), ios_base::binary); //quelle.c_str wandelt quelle zu C-String
	if(!ifQuelle) //Wenn Fehler im Strom vorliegt, gibt er false aus.
		return false;

	//initialisiere Ausgabestrom
	ofZiel.open((ziel.c_str()), ios_base::binary/*|ios_base::trunc|ios_base::app*/);
	if(!ofZiel)
		return false; // siehe oben
	return true;
}

string holeSchluessel()
{
	string temp;
	cout << "Bitte geben sie den Schlüssel zum (de)kodieren der Datei ein: \n";
	cin >> temp;
	if(temp.size()>MAX_SCHLUESSEL) //prüft Schlüsselgröße und fragt - falls nötig -
	{						   	   //erneut nach, bis der Schlüssel stimmt.
		cout << "Schlüssel zu groß!\n";
		return holeSchluessel();
	}
	return temp;
}

bool initSchluessel(string pSchluessel)
{
	sSchluessel = pSchluessel; 
	itAnfang = sSchluessel.begin();
	itEnde = sSchluessel.end();
	itGerade = sSchluessel.begin();
	return true;
}

//Verschluesselt mit Hilfe symetrischen Schlüssels:
//	*füllt den Puffer
//	*XOR Operation der einzelnen Charzeichen im Puffer/Schlüssel
//	*schreibt den Puffer in sZiel
//  *solange bis Datei komplett gelesen

bool verschluessele()
{	

	//Endlosschleife
	while(!ifQuelle.eof())
	{

		//den Puffer füllen
		int PufferStand = 1;
		cPuffer[0] = ifQuelle.get();
		while((!ifQuelle.eof()) && (PufferStand<MAX_PUFFER))
		{
			cPuffer[PufferStand] = ifQuelle.get();
			++PufferStand;
		}	

		//XOR Operation	
		for(int i =0; i<PufferStand; ++i)
		{
			cPuffer[i] = cPuffer[i]^(*itGerade);
			iteratorErhoehen();
		}

		//Puffer ins Ziel übertragen
		for(int i=0; i<=(PufferStand-1); ++i)
			{
				ofZiel << cPuffer[i];
		}
	}

	return true;			

}		

void iteratorErhoehen()
{
	if(itGerade!=(itEnde-1))
		++itGerade;
	if(itGerade==(itEnde-1))
		itGerade = itAnfang;
}

Das es noch nicht wirklich objektorientiert ist, ist mir irgendwann dann auch
aufgefallen.

Panke

Sieht doch für einen Anfänger gar nicht so schlecht aus, aber:

* holeSchlüssel() würde ich eher mit einer while-Schleife lösen:

string holeSchluessel()
{
    string temp;
    do
    {
        cout << "Bitte geben sie den Schlüssel zum (de)kodieren der Datei ein: \n";
        cin >> temp;
        if(temp.size()>MAX_SCHLUESSEL) //prüft Schlüsselgröße und fragt - falls nötig -
        {                                  //erneut nach, bis der Schlüssel stimmt.
            cout << "Schlüssel zu groß!\n";
        }
    while(temp.size()>MAX_SCHLUESSEL);
    return temp;
}

* iteratorErhöhen() schreit geradezu nach einem else (anstelle des zweiten if(...) )

* da du schon mit Strings arbeitest, kannst du doch auch von deren Speicherverwaltung profitieren - dann benötigst du auch die MAX.. Makros (in C++ sind übrigens "const int" üblicher) nicht mehr.

Den Kern-Algorithmus habe ich mir jetzt nicht genauer angesehen, aber er scheint OK zu sein.

MartinMilbret

Ich wette du hast schon mehrere programme geschrieben da ich selber noch anfänger bin wette ich das du schon andere testprogramme geschrieben hast.
Wenn das aber wirklich dein erstes ist dann respeckt

volkard

versuch mal, das programm so klein wie möglich zu kriegen. keine fehlerbehandlung. keine globalen variablen. ich fürchte, es wird hübscher als 6-zeiler aussehen. falls nicht, dann sollte dir kurze version trotzdem in mancherlei hinsicht vorbild sein.

terraner

wenn du schon globale variablen hast, dann tue sie mindestens in einen anonymen namespace. alles andere was mir auf den ersten blick aufgefallen ist, wurde schon gesagt.

Panke

Das erste das funktioniert und nicht aus irgendwelches Tuts ist.
Ich mach mich dann mal ans Kleinschreiben. Aber 6 Zeilen? Ob das geht ..

Welchen Vorteil bringt ein anonymer Namespace?

Caipi

Panke schrieb:

Welchen Vorteil bringt ein anonymer Namespace?

Objekte, die sich in einem anonymen Namespace befinden, haben internes Linkage. D.h. dass sie nur von der Übersetzungseinheit angesprochen werden kann, die den Namespace definiert.
Der anonyme Namespace hat den Vorteil, dass obwohl du innerhalb der ÜE ohne Qualifizierung auf Objekte, etc. in diesem Namensraum zugreifen kannst, diese von anderen ÜEs nicht angesprochen werden können.

Gruß Caipi

net 0

Panke schrieb:

Das erste das funktioniert und nicht aus irgendwelches Tuts ist.
Ich mach mich dann mal ans Kleinschreiben. Aber 6 Zeilen? Ob das geht ..

aber mach nicht den quelltext klein sondern sieh zu dass das was der compiler ausspuckt klein ist.

Hey Panke du bist ein Genie.

Taurin

Panke schrieb:

Das erste das funktioniert und nicht aus irgendwelches Tuts ist.
Ich mach mich dann mal ans Kleinschreiben. Aber 6 Zeilen? Ob das geht ..

Welchen Vorteil bringt ein anonymer Namespace?

6 Zeilen find ich auch schwierig, ohne jeglichen Stil aufzugeben. Aber ohne Fehlerabfragen komm ich auf 8 Zeilen mit allem drum und dran

volkard

Taurin schrieb:

6 Zeilen find ich auch schwierig, ohne jeglichen Stil aufzugeben. Aber ohne Fehlerabfragen komm ich auf 8 Zeilen mit allem drum und dran

das ist eine einsparung von 96% .
na, mal schauen, was Panke so schafft. und dann schauen, wie er das große programm mit ehlerabfragen und so zusammenklopfen mag. ich schätze mal, so 50% werden's.

Panke

Meint ihr alles in 8 Zeilen oder nur die Main Funktion?

Tante Edith:

Soo, gerade eben mal rumgetüftelt. Kleiner ist bei mir nicht drin:

#include <cstdlib>
#include <iostream>
#include <fstream>
#include <string>

using namespace std;

int main(int argc, char *argv[])
{
	cout << "Quelle: ";
	string quelle;
	cin  >> quelle;

	ifstream in(quelle.c_str(), ios_base::binary);

	cout << "Ziel: ";
	string ziel;
	cin >> ziel;

	ofstream out(ziel.c_str(), ios_base::binary);

	cout << "Key: ";
	string key;
	cin  >> key;

	int i=0;
	char c = in.get();
	while(!in.eof())
	{

		c = c^key[i];
		out << c;
		if(i<=key.size())
			++i;
		else i=0;
		c = in.get();
	}

    system("PAUSE");
    return EXIT_SUCCESS;
}

volkard

Panke schrieb:

Soo, gerade eben mal rumgetüftelt. Kleiner ist bei mir nicht drin:

uih! das ist aber hübsch geworden.

#include <cstdlib> //weg
#include <iostream>
#include <fstream>
#include <string>

using namespace std;

int main(int argc, char *argv[]) //woziu die parameter?
{
	cout << "Quelle: ";
	string quelle;
	cin  >> quelle;

	ifstream in(quelle.c_str(), ios_base::binary);

	cout << "Ziel: ";
	string ziel;
	cin >> ziel;

	ofstream out(ziel.c_str(), ios_base::binary);

	cout << "Key: ";
	string key;
	cin  >> key;

	int i=0; //aua
	char c = in.get(); //aua
	while(!in.eof())
	{

		c = c^key[i];
		out << c;
		if(i<=key.size())
			++i;
		else i=0;
		c = in.get();
	}

    system("PAUSE"); //weg
    return EXIT_SUCCESS;
}

irendwas an der schleife stört noch.
sagen wir mal, i sei nötig. und daß wir nicht den modulo-operator wollen una uch kein ?: oder -=bool(...).
dann ist

if(i<=key.size())
			++i;
		else i=0;

schon beinahe so gut wie

++i;
if(i==key.size())
   i=0;

beim enlesen nimmt man gerne

while(!in.get(c))

außerdem würde ichg nicht auf c rumschreiben, sondern einfach nur char(c^key[i]) ausgeben.

schauste mal, ob was von meiner neuerlichen mecker einbauebar ist?

und dann mußte halt schauen, ob du wirklich so stark reduzierten code gerne hast, oder leiber etwas mehr schreibst. vielleicht strebste ja auch mal eine stellung an, wo man nach anzahl der codezeilen entlohnt wird (soll's ja geben). oder willst spieleprogrammierer werden, da gehört codebloat zu den grundtugenden.

volkard

die 6 zeilen waren eine nicht sinnvoll erreichbare vorgabe. da hab ich etwas übertrieben, damit du in die richtige richtung läufst.

und wieviel zeilen sind sinnvoll?

Panke

++i;
if(i==key.size())
   i=0;

Nutze ich denn da das letzte Zeichen des Strings?

Wenn ich

system("Pause");

weglasse, schließt das Konsolenfenster immer sofort.
Ist der Befehl so grausam, dass sich da etwas eigenes schreiben lohnt?

Wenn deine If Abfrage funktioniert, könnt ich mir folgendes vorstellen, um das i lokaler zu halten:

for(int i = 0; !in.get(c); ++i)
{
    out << c^key[i];
    if(i==key.size()) //nicht eher i>key.size()?
        i=0;
}

Die Argumente stammen noch aus der Compilervorlage. Habe sie einfach nicht gelöscht.

Panke

??????????? schrieb:

und wieviel zeilen sind sinnvoll?

Schätze mal, Programm ist klein, schnell und erfüllt den erdachten Zweck ohne an Lesbarkeit zu verlieren.

Edit: Okay .. daneben

volkard

??????????? schrieb:

und wieviel zeilen sind sinnvoll?

besonders schöne programme haben höchstens 250 zeilen, das weiß ja jeder. man kann zufälligerweise besonders schöne programme nie in mehrere kleinere dateien aufteilen, so daß sets eine kleine datei eine zeile weniger als eine andere kleine datei hat. aber man kann besonders schöne programme auf x verschiedene weisen in kleinere dateien mit der gleichen zeilenanzahl aufteilen, wobei x immer gerade ist. ist das nicht seltsam?
das gesuchte programm ist ein besonders schönen programm. wenn ich dir nun sage, daß es auch besonders schöne programme gibt, die größer als das gesuchte programm sind und welche, die kleiner sind, isses klar, wieviele zeilen es hat.
viel spaß beim knobeln.

niemand

In Deiner Originalversion war genau an der Stelle ein Fehler den volkard korrigiert hat

out << c^key[i];
 if(i<key.size())  // i <= key.size() wäre falsch
    ++i;
 else i=0;

key[key.size()] ist wohl immer '\0' aber eigentlich nicht zulässig. Ausserdem macht das mit c^'\0' auch nicht so viel Sinn.

volkard hat diesen Fehler mit korrigiert:

out << c^key[i];
++i;
if(i==key.size())
   i=0;

Jetzt ist i immer kleiner key.size().

Ausserdem ist

i = (i + 1) % key.size();

noch kürzer, erfüllt den Zweck und ist schlechter lesbar...

Panke

Das c++ strings \0 terminiert sind, wusste ich gar nicht ..
Dann geht das natürlich so wie bei mir nicht, auch wenn es sich nicht wirklich ausgewirkt hat.

i = (i + 1) % key.size();

Dann doch lieber ein paar Zeilen mehr.

chris' 1

huhu,
@Panke:
womit hast du C++ gelernt/lernst du?