Konstruktive Kritik zu Vignère-Klasse

Caipi

Servus zusammen!
Habe mir eine Klasse zur Vignère-Verschluesselung geschrieben. Da ich aber noch ein totaler Anfänger bin, würde ich gerne mal wissen, was ich besser machen sollte bzw. verbessern könnte.

Soll ich z.B. komplett die Klasse-String verwenden und die char-Zeiger weglassen oder soll ich komplett mit dem Datentyp char arbeiten. (Dafür würde ich jetzt plädieren, da ich die String-Klasse nicht so mag ).

Naja, lange Rede kurzer Sinn, gibt mir doch einfach mal ein wenig Kritik zu der Klasse.

Hier der Code:

#ifndef _VIGNERE_H_
#define _VIGNERE_H_

#include <string>
using namespace std;

const char alpha[27] = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '\0' };

class Vignere
{
        private:
                char *plainText;   // Klartext
                char *cipher;      // Cipher
                char *keyWord;     // Schluessel

        public:
                Vignere();
                Vignere(const string& tempText, const string& tempKeyWord);
                ~Vignere();

                void receiveData(const string& tempText, const string& tempKeyWord);
                int searchCharPos(const char& searchChar);
                void crypt();
                void encrypt();
                int getSize(); // Ermittelt die Groesse des Ver/Entschluesselten Textes.
                void getPlainText(char *pPlainText); // BETA
                void getCipher(char *pCipher); // BETA
};                                

Vignere::Vignere()
{
        // Keinen Speicher belegen und auf NULL zeigen.
        plainText   = NULL;
        cipher      = NULL;
        keyWord     = NULL;
}

Vignere::Vignere(const string& tempText, const string& tempKeyWord)
{
        // Speicher dynamisch allozieren...
        plainText = new char[tempText.length() + 1];  // + 1 wegen \0 (Stringende)
        cipher = new char[tempText.length() + 1];     //            " 
        keyWord = new char[tempKeyWord.length() + 1]; //            "

        // ..., bei plainText und cipher zuweisen
        for(unsigned int i = 0; i < tempText.length(); i++)
        {
                plainText[i] = tolower(tempText[i]);
                cipher[i] = tolower(tempText[i]);
        }        

        // ... und bei keyWord zuweisen.        
        for(unsigned int i = 0; i < tempKeyWord.length(); i++)
                keyWord[i] = tolower(tempKeyWord[i]);

        // Stringende-Zeichen zuweisen.        
        plainText[tempText.length()] = '\0';
        cipher[tempText.length()] = '\0';
        keyWord[tempKeyWord.length()] = '\0';          
}

Vignere::~Vignere()
{
        // Speicher freigeben und auf NULL zeigen.
        delete[] plainText;
        plainText = NULL;
        delete[] cipher;
        cipher = NULL;
        delete[] keyWord;
        keyWord = NULL;
}                        

void Vignere::receiveData(const string& tempText, const string& tempKeyWord)
{
        // Speicher dynamisch allozieren...
        plainText = new char[tempText.length() + 1];  // + 1 wegen \0 (Stringende)
        cipher = new char[tempText.length() + 1];     //            " 
        keyWord = new char[tempKeyWord.length() + 1]; //            "

        // ..., bei plainText und cipher zuweisen
        for(unsigned int i = 0; i < tempText.length(); i++)
        {
                plainText[i] = tolower(tempText[i]);
                cipher[i] = tolower(tempText[i]);
        }        

        // ... und bei keyWord zuweisen.        
        for(unsigned int i = 0; i < tempKeyWord.length(); i++)
                keyWord[i] = tolower(tempKeyWord[i]);

        // Stringende-Zeichen zuweisen.        
        plainText[tempText.length()] = '\0';
        cipher[tempText.length()] = '\0';
        keyWord[tempKeyWord.length()] = '\0';       
}           

int Vignere::searchCharPos(const char& searchChar)
{
        for(unsigned int i = 0; i < strlen(alpha); i++)
        {
                if(searchChar == alpha[i] || searchChar == toupper(alpha[i]))
                        return i;
        }
        return -1;
}                             

void Vignere::crypt()
{       
        // Schleife wird fuer jeden Klarbuchstaben ausgefuehrt. Daher: jeder 
        // Buchstabe wird seperat verschluesselt. 
        for(unsigned int i = 0; i < strlen(plainText); i++)
        {
                // Fuer den aktuellen Buchstaben den Index im Array alpha suchen und speichern,
                // gleiches gilt fuer den aktuellen Schluesselbuchstaben.
                int plainTextNr = searchCharPos(plainText[i]);
                int keyWordNr = searchCharPos(keyWord[i]);

                // Sofern die Summe der Indexe kleiner als 26 wird der zu verschluesselnde
                // Buchstabe durch die Summe der beiden Indexe verschluesselt. 
                if(plainTextNr + keyWordNr < 26)
                        cipher[i] = alpha[plainTextNr + keyWordNr];

                // Ansonsten wird der Buchstabe aus der Summe der beiden Indexe minus
                // 26 verschluesselt. Dies hat den Sinn, damit wieder bei A angefangen wird. (siehe Vignere-Kasten)         
                else
                        cipher[i] = alpha[plainTextNr + keyWordNr - 26]; // Wieder bei A anfangen.
        }                                       
}                                                        

void Vignere::encrypt()
{
        // Schleife wird fuer jeden Verschluesselten Buchstaben ausgefuehrt. Daher:
        // jeder Buchstabe wird seperat entschluesselt.
        for(unsigned int i = 0; i < strlen(cipher); i++)
        {       
                // Fuer den aktuellen Buchstaben den Index im Array alpha suchen und speichern,
                // gleiches gilt fuer den aktuellen Schluesselwortbuchstaben.
                int cipherNr = searchCharPos(cipher[i]);
                int keyWordNr = searchCharPos(keyWord[i]);

                // Sofern die Differenz der Indexe groesser oder gleich null, den Buchstaben
                // entschluesseln durch die Differenz der beiden Indexe.
                if(cipherNr - keyWordNr >= 0)
                        plainText[i] = alpha[cipherNr - keyWordNr];

                // Ansonsten wird der Buchstabe aus der Differenz der beiden Indexe plus 26
                // entschluesselt. Dies hat den Sinn, damit bei Z weitergemacht wird. (siehe Vignere-Kasten)        
                else
                        plainText[i] = alpha[cipherNr - keyWordNr + 26]; // bei Z weitermachen.
        }                 
}        

// Ermittelt die Groesse des Klartextes/Ciphers.
// Mithilfe dieser Groesse wird Speicherplatz reseviert.
int Vignere::getSize()
{
        return strlen(plainText); // ... oder strlen(cipher), da am Anfang die Daten gleichgesetzt werden.
}                                   

// Klartext in Zeiger *pPlainText kopieren und somit als Kopie 
// (z.B.) in Main zurueckgeben
void Vignere::getPlainText(char *pPlainText)
{
        for(unsigned int i = 0; i < strlen(cipher); i++)
                pPlainText[i] = plainText[i];     
}

// Cipher in Zeiger *pCipher kopieren und somit als Kopie 
// (z.B.) in Main zurueckgeben
void Vignere::getCipher(char *pCipher)
{
        for(unsigned int i = 0; i < strlen(cipher); i++)
                pCipher[i] = cipher[i];        
}

#endif // _VIGNERE_H_

Caipi

rüdiger

ich hab das jetzt nur grob überflogen. Aber was mir sofort ins Auge springt

#ifndef _VIGNERE_H_

Unterstrich am Anfang mit Großbuchstaben im globalen Namespace sind nicht erlaubt! (siehe FAQ)

using namespace macht man in Headern idr. nicht! alpha gehört nicht in den globalen Namespace. ZB. als static-Konstante in die Klasse.

Das Design deiner Klasse finde ich strange, dazu äußere ich mich aber später.

std::string und C-Strings würde ich nicht mischen, dass sollte idr. entsprechend Performance kosten und unnötige komplexität in den Code bringen. Vorallem die Speicherverwaltung würde ich extern regeln.

Und teilweise benutzt du int wo du lieber std::size_t oä benutzen solltest.

ness

-strings sind allgemein einfacher zu benutzen als zeiger auf char, sie sind weniger anfällig für resourcenlecks und verfügen über eine Funktion .c_str()/.data(), mit denen man ihre "BAusteine" ausgeben lassen kann -> Eigentlich fast nur pos.
-NULL ist verpönt, in c++ auch nichts anderes als 0
Hab jetzt nur den Anfang gelesen..., sieht aber (In den Augen eines Anfängers) sauber aus...
/edit: Die Klasse an sich ist oop-technisch net so ganz gelungen?

Ben04

Soll ich z.B. komplett die Klasse-String verwenden und die char-Zeiger weglassen oder soll ich komplett mit dem Datentyp char arbeiten.

std::string bis zur Vergasung.

const char alpha[27] = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '\0' };

Hier fände ich

const char alpha[] = "abcdefghijklmnopqrstuvwxyz";

besser.

char *cipher;      // Cipher

Nichtssagende Kommentare erhöhen nicht die Lesbarkeit.

int getSize();

Eigentlich ist kingrudis Job aber Größenangaben bitte in size_t oder unsigned da sie ja nicht negativ sein können.
EDIT: Da war kingrudi doch schneller

pPlainText[i] = plainText[i];

Entweder konsequent typeFoo oder foo aber nicht mischen. Hier muss man ja drei mal hinschauen um zu sehen, dass es 2 verschiedene Namen sind.

void Vignere::getPlainText(char *pPlainText)

Mach dir das Leben leicht und gib ein std::string zurück.

Caipi

@ness

/edit: Die Klasse an sich ist oop-technisch net so ganz gelungen?

Was meinst du damit?

An alle anderen. Danke schonmal, aber vielleicht kommt ja noch mehr.

Caipi

ness

Du versuchst bei oop doch dinge abzubilden. Ich weiß net ob das richtig ist, aber für mich wäre eine Klasse string_to_vigner und eine Funktion vigner(string_to_vigner& v) Aber vielleicht seh ich das ja falsch?

otze

Vignere::Vignere()
{
        // Keinen Speicher belegen und auf NULL zeigen.
        plainText   = NULL;
        cipher      = NULL;
        keyWord     = NULL;
}

definitiv unnütz, du fragst 0 eh nicht ab. Da kann das teil gleich leer sein

dein 2. konstruktor tut genau das gleiche wie receiveData, wieso receive dann nicht innerhalb des ctors benutzen?

in receivedata fragst du nicht ab, ob bereits ein wert in der Klasse enthalten ist, und überschreibst den pointer,ohne die alten werte gegebenenfalls wieder freizugeben. das darf nicht passieren.

nehmen wir mal an, ich würde mir den code nicht näher anschauen, sondern nur die klasse verwenden: was ist der unterschied zwischen crypt und encrypt?

es fehlt definitiv die entschlüsselungsmethode, so ein Filter sollte immer in 2 richtungen funktionieren.

welchen string durchsucht getCharPos eingabe,schlüssel oder ausgabe? Wird das erste, das zweite, das dritte oder letzte vorkommen des buchstaben im string zurückgegeben? was hat die methode überhaupt für einen sinn?
//edit ahh, du fragst einen wert ab, der in einem globalen array ausserhalb der Klasse existiert. Geschickt. Wirklich. und so selbsterklärend.

wieso bist du so kommentierwütig?
und was zum geier ist ein Cipher?
fragen über fragen

ness

otze schrieb:

wieso bist du so kommentierwütig?

Ein glück das ich hier nie was zeige...

Caipi

@otze

definitiv unnütz, du fragst 0 eh nicht ab. Da kann das teil gleich leer sein

Ich dachte wenn man einen Zeiger nicht definitiv auf NULL zeigen lässt, zeigt er irgendwohin, und ist somit ein "wilder" Zeiger.

dein 2. konstruktor tut genau das gleiche wie receiveData, wieso receive dann nicht innerhalb des ctors benutzen?

Weil ich (manchmal) zweimal Daten mit der gleichen Instanz empfangen will. Dann benutze ich bei der Definition den 2. Konstruktor und wenn ich dann noch mal Daten empfangen möchte, die Instanz aber schon definiert ist, benutze ich dafür die Funktion receiveData(...). Vergleichbar mit ifstream data("file.txt") und data.open("file.txt").

in receivedata fragst du nicht ab, ob bereits ein wert in der Klasse enthalten ist, und überschreibst den pointer,ohne die alten werte gegebenenfalls wieder freizugeben. das darf nicht passieren.

Da hast du recht! Ist mir bisher gar nicht aufgefallen. Danke!

nehmen wir mal an, ich würde mir den code nicht näher anschauen, sondern nur die klasse verwenden: was ist der unterschied zwischen crypt und encrypt?

es fehlt definitiv die entschlüsselungsmethode, so ein Filter sollte immer in 2 richtungen funktionieren.

Die Entschluesselungsmethode fehlt definitiv nicht. Schau sie dir noch mal genau an. crypt() verschluesselt. encrypt() entschluesselt (Rechenweg ist genau umgekehrt)

welchen string durchsucht getCharPos eingabe,schlüssel oder ausgabe? Wird das erste, das zweite, das dritte oder letzte vorkommen des buchstaben im string zurückgegeben? was hat die methode überhaupt für einen sinn?

Die Funktion getCharPos(char& charPos) ermittelt das aktuell zu ver/entschluesselnde Zeichen aus dem Array alpha[] und liefert den Index zurueck. Das habe ich gemacht, damit ich den Index des aktuellen Zeichens bekomme, welcher wiederum Teil der Ver/Entschluesselung ist.

wieso bist du so kommentierwütig?
und was zum geier ist ein Cipher?

Eigentlich hasse ich das kommentieren. Ich habe aber früher schonmal die Caesar-Verschluesselung geschrieben (ohne Kommentare). Im nachhinein ist es mir dann ganz schön schwer gefallen. Nochmal den ganzen Code zu verstehen

Achso, und Cipher bedeutet Chiffre, Rätsel.

Caipi

ness

Caipi schrieb:

@otze

definitiv unnütz, du fragst 0 eh nicht ab. Da kann das teil gleich leer sein

Ich dachte wenn man einen Zeiger nicht definitiv auf NULL zeigen lässt, zeigt er irgendwohin, und ist somit ein "wilder" Zeiger.

Nein, dann zeigt er *nirgendwo* hin...

otze

Weil ich (manchmal) zweimal Daten mit der gleichen Instanz empfangen will. Dann benutze ich bei der Definition den 2. Konstruktor und wenn ich dann noch mal Daten empfangen möchte, die Instanz aber schon definiert ist, benutze ich dafür die Funktion receiveData(...). Vergleichbar mit ifstream data("file.txt") und data.open("file.txt").

erstens: file.open(...) ist ein designfehler, das haben die entwickler selber zugegeben.
zweitens: worauf ich hinauswollte, ist wieso du die funktion receiveData nicht im konstruktor aufrufst um die Werte zu vergeben? du schreibst 2mal den komplett gleichen code. Das ganze hätte ein Oneliner werden können

Ich dachte wenn man einen Zeiger nicht definitiv auf NULL zeigen lässt, zeigt er irgendwohin, und ist somit ein "wilder" Zeiger.

dass er nirgendwohinzeigt ist zutreffender, aber wieso setzt du die zeiger auf null, wenn du nirgendwo in deinem gesamten code prüfst ob die zeiger 0 sind.
würde ich zb sowas machen:

Vignere x;
x.crypt();

dann würde das gesamte programm als riesiger wunderschöner Feuerball im Datennirvana aufgehen, eben weil nichts prüft, ob für den zeiger schon speicher allokiert wurde(imho ein vorteil von std::string, da kann sowas nie passieren).
eine ähnliche sache im dtor: da setzt du zeiger noch umständlich auf 0, obwohl das objekt gleich eh nurnoch asche ist

getCharPos ist ein einziger designfehler, das ist besser als funktion implementiert, da es nichts direkt mit der Klasse zu tun hat, die getCharPos aufruft. Alles was wichtig für getCharPos ist, ist ausserhalb, also gehörts nicht in die Klasse.

gegen kommentare hat hier niemand etwas, aber die meisten kommentare sind bei dir a) redundant, mir ist spätestens nach dem ersten kommentar klar,dass da ein +1 beim string allokieren fürs "\0" Byte drangehört ;). Auch so sachen wie

char *plainText;   // Klartext
char *cipher;      // Cipher
char *keyWord;     // Schluessel

oder sachen, in denen jede zeile kommentiert wird, wenn ein kurzer absatz ausgereicht hätte, haben insgesamt betrachtet einen informationswert, den ich nahe an der Höhe des Meeresspiegels ansiedeln würde

rüdiger

zu crypt/encrypt: Encrypt heißt nicht Entschlüsseln. Encrypt heißt Verschlüsseln. Decrypt ist Entschlüsseln.

http://dict.leo.org/?lp=ende&lang=de&searchLoc=0&cmpType=relaxed&relink=on&sectHdr=on&spellToler=std&search=decrypt

Und wie ness bereits gesagt hat, sollten Ver-/Ent-schlüsselung an sich einfache Funktionen sein, für den Schlüssel lohnt sich eher eine Klasse

namespace crypt {
  namespace vingere {
    class key {
      const char *key_m;
    public:
      //...
    };
    template<typename Iterator>
    void encrypt(Itertor &begin, Iterator &end, key k);
    template<typename Iterator>
    void decrypt(Itertor &begin, Iterator &end, key k);
  }
}

Caipi

Danke für die ganze Kritik.
Habe die Klasse (zum größten Teil) nach eurer Kritik überarbeitet. Ist auf jeden Fall besser jetzt.

Caipi