Konstruktive Kritik zu Vignère-Klasse

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