@blub²,
Du meinst wohl eher drakon, er hat dies gesagt
@shikari*,
Also hier mal ein paar Kommentare. Ich hoffe, dass ich das meiste erwischt habe. Den Algorithmus an sich habe ich nicht korrigiert, habe nur Tipps im Code gegeben für eine bessere Struktur.
#include <string> // fehlt, für std::string
#include <iostream>
#include <algorithm> // braucht man, falls man ein paar Dinge schöner machen möchte.
// Dazu später...
using namespace std; // Pass damit auf. Man sollte
// using namespace nur mit äusserster Vorsicht einsetzen.
// Du heblst hier die Funktion des Namensraumes aus. Wenn du
// sowas nur machst, weil du zu faul bist std:: zu schreiben,
// dann ist hier wirklich was verkehrt.
// Herrje. Wieso ein Makro, wenn es auch eine Funktion tun würde?
#define Xor(X,Y) (!!(X) ^ !!(Y))
// Makros sind sehr böse, da hinter dem Makro nur ein dummer
// Textersetzungsmechanismus steckt. Makrocode kümmert sich einen
// Scheiss um C++ Quellcode. Dadurch können sehr seltsame Fehler
// entstehen. Makros werden vor allem in C als Ersatz für die Templates
// in C++ benutzt. Oder vielleicht so formuliert, damit kein C'ler wütend wird:
// Viele Makros, welche in C eingesetzt werden, können in C++ durch
// Templates ersetzt werden.
string RC4(string text, string password);
int main() {
// Geben wir dem Code doch etwas Platz ;)
// string sText, sPassword, sReturn;
// Man muss nicht immer alle Variablen ganz oben deklarieren.
// Das galt nur bei den alten C Standards, in C++ war es nie der Fall.
// Wegen der Übersichtlichkeit deklariert man die Variablen
// heutzutage dort, wo man sie auch gleich als erstes braucht.
cout << "Text:" << endl;
string sText;
cin >> sText;
// Was passiert bei falscher Eingabe?
// Zum Beispiel statt eines Wortes, mehrere.
cout << "Password:" << endl;
string sPassowrd;
cin >> sPassword;
// Was passiert bei falscher Eingabe?
string sReturn = RC4(sText, sPassword);
cout << sText.at(5) << endl; // Debug und je nach dem löst es eine Exception aus.
cout << sReturn;
return 0;
}
string RC4(string const& text, string const& password) {
// Die Referenz auf ein konstantes Objekt vermeidet unnötige
// Kopien. Vor allem bei komplexen und grosse Objekten,
// sollte man statt einer Kopie eine Referenz auf ein konstantes
// Objekt übergeben. Ausser man benötigt wirklich eine Kopie ;)
// Bei fundamentalen Datentypen, bzw. kleinen Datentypen,
// sollte man dagegen immer eine Kopie nehmen, da die Kopie
// dann besser ist, bzw. die Referenz mehr Nachteile hat.
int RB[255]; // Der Name sagt nichts.
long x, y, z; // Die Namen sagen nichts.
unsigned char key[255];
//unsigned char ByteArray[text.length()]; // Kein C++ Standard. Unter GCC geht es wahrscheinlich,
// da GCC eine entsprechende Erweiterung unterstütz,
// kommend aus C99. Aber wie gesagt, ist nicht Standard
// kompatibel.
// In C++ solltest du wohl sowas machen:
std::vector<unsigned char> ByteArray(text.length());
// Dafür brauchst du aber den Header <vector>
// unsigned char Temp; <- wird gar nicht mehr benötigt.
string RC4;
// Wieder gilt, dass man nicht alle Variablen ganz oben deklarieren muss.
// Ich bin jetzt aber zu faul, diese auch runterzuschieben. Du sollst ja eigentlich
// den Code umschreiben und verbessern, nicht ich :)
cout << "1" << endl; // Debug
if(text.length() == 0) {
return NULL;
// NULL ist ein Makro, welches durch 0 ersetzt wird.
// Dadurch wird ein std::string(0) erstellt.
// Was eigentlich ein std::string((char const*)0); ist.
// Was schlussendlich undefiniert ist.
// Korrekt wäre hier:
// return "";
// oder
// return std::string();
// oder
// return RC4;
// RC4 ist hier ja ein leerer std::string.
}
else if(password.length() == 0) {
return NULL;
// Siehe oben.
}
cout<< "1" << endl; // Debug
for(int i = 0; i < 255; i++) {
key[i] = password.at(i); // Was passiert wenn password wenig als 255 Zeichen hat?
} // Eine Exception kommt geflogen ... la la la la la laaaaa
// Und was ist, wenn password mehr Zeichen hat? Also irgendwas stimmt in diesem Algo nicht ;)
// Auch könnte man diese for-Schleife mit einem std::copy verkürzen und lesbarer machen.
// Ein Beispiel wie man std::copy aus dem Header <algorithm> verwendet, kommt später.
cout<< "1" << endl;
for(int i = 0; i < 255; i++) {
RB[i] = i;
}
cout <<"1" << endl; // Debug
x = 0;
y = 0;
z = 0;
for(/*x = 0*/; x < 255; x++) { // schon wieder x = 0? Ist doch schon 0!
// Oje, sehr übersichtlicher Code folgt. Vor allem kombiniert mit den
// unleserlichen Variablennamen.
// Hier wäre es wohl sinnvoll Dinge in Funktionen auszulagern.
// Oder zumindest mit Zwischenresultaten zu arbeiten.
y = ((y + RB[x] + key[x % password.length()]) % 256);
// Auch für das nächste wäre eine Funktion was nettes. Es gibt
// sogar bereits eine in der Standardbibliothek. Dafür muss
// aber der Header <algorithm> eingebunden werden.
/*
Temp = RB[x];
RB[y] = RB[y];
RB[y] = Temp;
*/
std::swap(RB[y], RB[x]);
// Ist deutlich deutlich lesbarer so eine Funktion, nicht?
}
x = 0;
y = 0;
z = 0;
cout<< "1" << endl; // Debug
// Das folgende kann man auch vereinfachen:
/*for(int i = 0; i < text.length(); i++) {
ByteArray[i] = text.at(i);
} */
std::copy(text.begin(), text.end(), ByteArray.begin());
// Dafür ist der Header <algorithm> nötig.
for(/*x = 0*/; x < text.length(); x++) { // Wieder setzt du x = 0, obwohl x schon 0 ist :)
// Wieder sehr übersichtlicher Code. Immerhin mit ein paar Zwischenergebnissen.
// Aber durch die schlechten Namen...
y = (y + 1) % 256;
z = (z + RB[y]) % 256;
// Kann man wieder kürzen dank swap aus <algorithm>
/*
Temp = RB[y];
RB[y] = RB[z];
RB[z] = Temp;
*/
std::swap(RB[y], RB[x]);
// Tjo und das ist auch nicht gerade übersichtlich und
// der Übersichtlichkeit hilft das Makro nicht. Auch stimmt
// der Name nicht so wirklich. Es ist kein reines XOR.
ByteArray[x] = (Xor (ByteArray[x], (RB[RB[y] + RB[z]] % 256)));
}
/*
for(int i = 0; i < text.length(); i++) {
RC4 += ByteArray[i];
}
*/
// Kann man wieder kürzen:
std::copy(ByteArray.begin(), ByteArray.end(), RC4.begin());
// Wieso arbeitest du allerdings nicht direkt auf dem RC4 Objekt?
// Könntest am Anfang statt:
// std::vector<unsigned char> ByteArray(text.length());
// std::string RC4;
// Sowas machen:
// std::string RC4 = text;
// So würde auch die erste Kopie von text nach ByteArray wegfallen.
cout <<"1" << endl; // Debug
return RC4;
}
Wenn man die Korrekturen angewendet hat, könnte man wahrscheinlich nochmals drüber gehen
Ein paar allgemeine Sachen:
- Verwende die Standardbibliothek. Gerade für Container und Algorithmen ist sie extrem praktisch. Eine fast vollständige Referenz findest du hier:
http://www.cplusplus.com/reference/
- Verzichte auf Makros. Daher verwende lieber auch kein INT_MIN, sondern nehme das std::numeric_limits<int>::min(). Vielleicht ein wenig übertrieben, aber gerade als Anfänger, sollte man sich manchmal ganz klare Grenzen machen. Später kann man diese immer noch lockern, verschärfen ist aber immer problematisch.
- Probier nicht so lange aus, bis etwas kompiliert. Sondern informier dich darüber und wende die Sache so an, wie man sie anwenden muss. Der Kompiler kompiliert noch viel, was nicht korrekt ist. In C und C++ gibt es das "undefinierte Verhalten" (auch UB abgekürzt für Undefined Behavior). Es ist eine korrekte Syntax, der Kompiler lässt es somit durch, aber das Resultat ist undefiniert. Daher sollte man sich nicht zu sehr darauf verlassen, dass etwas korrekt ist, nur weil der Kompiler es aktzeptiert
Grüssli
PS: LoL, der Beitrag wurde so lang, dass es zu Problemen mit dem Forum kam
