Bitte um Refactoring, Passwortgenerator mit Vorgaben

[EinNutzer0]

Huhu, bitte schaut hier mal rein, was man verbessern könnte.

Vorgaben:

• 30 zufällig gewählte Zeichen (die Wahrscheinlichkeit für zum Beispiel Ziffern soll nicht höher sein als Buchstaben...)
• Min. 1 Kleinbuchstabe
• Min. 6 Großbuchstaben
• Min. 8 Ziffern
• 2 bis 5 Sonderzeichen (aus '!' bis '/')

#include <stdexcept>
#include <iostream>
#include <random>
#include <vector>
#include <algorithm>

#define SIZEC 30

using namespace std;

class mrand
{
private:
    random_device rd;
    mt19937 gen = mt19937(rd());

public:
    int get_rand_int_max(int max)
    {
        uniform_int_distribution<> distr(0, max - 1);
        return distr(gen);
    }
};

mrand mr = mrand();

class alphabet
{
private:
    vector<char16_t> all_chars;
    size_t min;
    size_t max;

public:
    alphabet(char16_t from, char16_t to, size_t min, size_t max);
    char16_t get_random_char();
    int search_char(string s);
    int is_valid(string s);
    int get_size();
    char16_t get_char(int i);
};

class alphabets
{
private:
    vector<alphabet> all_alphabets;
    size_t all_size = 0;

public:
    alphabets();
    char16_t get_random_overall_char();
    string generate_valid_password();
    void validate(string s);
};

alphabet::alphabet(char16_t from, char16_t to, size_t min, size_t max)
{
    for (char16_t i = from; i <= to; i++)
    {
        all_chars.push_back(i);
    }
    this->min = min;
    this->max = max;
}

char16_t alphabet::get_random_char()
{
    return all_chars[mr.get_rand_int_max(all_chars.size())];
}

int alphabet::search_char(string s)
{
    int i;
    do
    {
        i = mr.get_rand_int_max(s.size());
    } while (find(all_chars.begin(), all_chars.end(), s[i]) == all_chars.end());
    return i;
}

int alphabet::is_valid(string s)
{
    int c = 0;
    for (auto i : s)
    {
        if (find(all_chars.begin(), all_chars.end(), i) != all_chars.end())
        {
            c++;
        }
    }
    return c < (int)min ? -1 : (c > (int)max ? 1 : 0);
}

int alphabet::get_size()
{
    return all_chars.size();
}

char16_t alphabet::get_char(int i)
{
    return all_chars[i];
}

alphabets::alphabets()
{
    all_alphabets.push_back(alphabet('a', 'z', 1, SIZEC));
    all_alphabets.push_back(alphabet('A', 'Z', 6, SIZEC));
    all_alphabets.push_back(alphabet('0', '9', 8, SIZEC));
    all_alphabets.push_back(alphabet('!', '/', 2, 5));
    for (auto a : all_alphabets)
    {
        all_size += a.get_size();
    }
}

char16_t alphabets::get_random_overall_char()
{
    int i = mr.get_rand_int_max(all_size);
    for (auto a : all_alphabets)
    {
        if (i < a.get_size())
        {
            return a.get_char(i);
        }
        i -= a.get_size();
    }
    return 0;
}

string alphabets::generate_valid_password()
{
    string s = "";
    while (s.size() < SIZEC)
    {
        s += (char)get_random_overall_char();
    }
    validate(s);
    return s;
}

void alphabets::validate(string s)
{
    bool f = true;
    while (f)
    {
        f = false;
        for (auto a : all_alphabets)
        {
            if (a.is_valid(s) != 0)
            {
                f = true;
                int v;
                while ((v = a.is_valid(s)) != 0)
                {
                    if (v < 0)
                    {
                        s[mr.get_rand_int_max(s.size())] = (char)a.get_random_char();
                    }
                    else
                    {
                        s[a.search_char(s)] = get_random_overall_char();
                    }
                }
            }
        }
    }
}

int main()
{
    alphabets a = alphabets();
    cout << a.generate_valid_password();
    cout << endl;
    return 0;
}

[Quiche Lorraine]

@EinNutzer0 sagte in Passwort generator mit Bedingungen:

int get_rand_int_max(int max)
{
uniform_int_distribution<> distr(0, max - 1);
return distr(gen);
}

Sieht ein wenig redundant aus. Pro Zufallszahl legst du eine uniform_int_distribution<> Instanz an.

Probiere doch mal eine Lösung mittels std::generate_n, std::back_inserter und std::shuffle.

Hierzu habe ich eine kleine Hilfsklasse benötigt, welche mir den Zeichensatz (z.B. "ABC...") und die Anzahl der zu erzeugenden Zeichen speichert und mittels dem Operator () für std::generate_n erlaubt, zufällig ein Zeichen aus der Zeichenkette zu wählen. Dann ein std::vector über die 4 verschiedenen Zeichensätze definieren...

[Schlangenmensch]

@EinNutzer0 sagte in Bitte um Refactoring, Passwortgenerator mit Vorgaben:

get_random_overall_char

Noch ein Thread dazu...

Deine void alphabets::validate(string s) ist ein Ungeheuer. Kannst du erklären, warum die was wie macht und was dazu wofür nötig ist?

Sehe ich das richtig, die validate() bekommt eine Kopie, arbeitet auf der und schmeißt die weg?

Warum manchmal this-> und manchmal ohne?

Sonst gelten meine Anmerkungen aus dem anderen Thread immer noch.

[EinNutzer0]

Danke für eure Hinweise.

@EinNutzer0 sagte in Bitte um Refactoring, Passwortgenerator mit Vorgaben:

validate(s);

Ich hatte angenommen, dann könnte ich in validate s beliebig verändern... (Also, dass Stringreferenzen übergeben werden)

[Schlangenmensch]

@EinNutzer0 Die Signatur für eine String Referenz sieht so aus:

void validate(string& s);

[wob]

@Quiche-Lorraine sagte in Bitte um Refactoring, Passwortgenerator mit Vorgaben:

Sieht ein wenig redundant aus. Pro Zufallszahl legst du eine uniform_int_distribution<> Instanz an.

Das macht nichts. Ich habe irgendwo mal einen Benchmark gesehen, dass es sogar effizienter ist, die Distribution immer neu anzulegen als sie zu speichern. Das Objekt wird wohl in der Regel wegoptimiert. (könnte man natürlich nochmal profilen/überprüfen, aber ich würde mir darüber erstmal keine Gedanken machen)

Ich würde mich eher fragen, ob die gesamte mrand-Klasse wirklich nötig ist. Wird der Code dadurch wirklich besser/einfacher, zumal dieses mr-Objekt nun global ist. Ich würde dem get_random_char (und anderen Funktionen) einfach einen URBG&& rng-Parameter spendieren wie ihn auch std::shuffle hat.

[EinNutzer0]

@Schlangenmensch sagte in Bitte um Refactoring, Passwortgenerator mit Vorgaben:

@EinNutzer0 Die Signatur für eine String Referenz sieht so aus:

void validate(string& s);

Anscheinend hatte ich den string s "by value" übergeben, und nicht "by reference": https://stackoverflow.com/questions/7626116/how-to-pass-object-to-function-in-c

Jetzt frage ich mich gerade, weshalb ich den innerhalb von validate verändern konnte? Compiler: g++

"flache" Kopie?

Sorry, aber scheinbar bekomme ich ja nicht mal die einfachsten Dinge hin...

[SeppJ]

Wie kommst du zu der Annahme, du hättest den String geändert?

[EinNutzer0]

Naja, das Programm würde andernfalls nicht anhalten.

[SeppJ]

Ich könnte jetzt ja weiter hypothetische Fragen stellen, wieso du denkst, dass das Programm nicht anhalten würde, aber sparen wir uns das und kommen direkt zu der Schlussfolgerung, dass du schlichtweg nicht weißt, was eine Referenz ist, oder was überhaupt das Problem ist, das Schlangenmensch beschrieben hat. Da weiß ich nicht mehr weiter zu helfen, außer Verweis auf die absoluten Grundlagen von C++. Dir wird hier niemand die ersten 4 Kapitel eines Lehrbuchs zitieren wollen. Das kannst du ganz einfach selber lernen. Und solltest du, denn das ist ungeheuer wichtig. Und in den restlichen Kapiteln steht bestimmt auch viel nützliches.

[EinNutzer0]

@SeppJ Bitte sei doch nicht so unfreundlich. Ja, ich habe falsch gedacht. Ich ging davon aus, ohne es zu überprüfen, dass das Programm das Richtige tun würde...

Bitte übersetzt und startet mal folgendes Programm:

#include <stdexcept>
#include <iostream>
#include <random>
#include <vector>
#include <algorithm>

#define SIZEC 30

using namespace std;

class mrand
{
private:
    mt19937 gen = mt19937(12);

public:
    int get_rand_int_max(int max)
    {
        uniform_int_distribution<> distr(0, max - 1);
        return distr(gen);
    }
};

mrand mr = mrand();

class alphabet
{
private:
    vector<char16_t> all_chars;
    size_t min;
    size_t max;

public:
    alphabet(char16_t from, char16_t to, size_t min, size_t max);
    char16_t get_random_char();
    int search_char(string s);
    int is_valid(string s);
    int get_size();
    char16_t get_char(int i);
};

class alphabets
{
private:
    vector<alphabet> all_alphabets;
    size_t all_size = 0;

public:
    alphabets();
    char16_t get_random_overall_char();
    string generate_valid_password();
    void validate(string s);
};

alphabet::alphabet(char16_t from, char16_t to, size_t min, size_t max)
{
    for (char16_t i = from; i <= to; i++)
    {
        all_chars.push_back(i);
    }
    this->min = min;
    this->max = max;
}

char16_t alphabet::get_random_char()
{
    return all_chars[mr.get_rand_int_max(all_chars.size())];
}

int alphabet::search_char(string s)
{
    int i;
    do
    {
        i = mr.get_rand_int_max(s.size());
    } while (find(all_chars.begin(), all_chars.end(), s[i]) == all_chars.end());
    return i;
}

int alphabet::is_valid(string s)
{
    int c = 0;
    for (auto i : s)
    {
        if (find(all_chars.begin(), all_chars.end(), i) != all_chars.end())
        {
            c++;
        }
    }
    return c < (int)min ? -1 : (c > (int)max ? 1 : 0);
}

int alphabet::get_size()
{
    return all_chars.size();
}

char16_t alphabet::get_char(int i)
{
    return all_chars[i];
}

alphabets::alphabets()
{
    all_alphabets.push_back(alphabet('a', 'z', 1, SIZEC));
    all_alphabets.push_back(alphabet('A', 'Z', 6, SIZEC));
    all_alphabets.push_back(alphabet('0', '9', 8, SIZEC));
    all_alphabets.push_back(alphabet('!', '/', 2, 5));
    for (auto a : all_alphabets)
    {
        all_size += a.get_size();
    }
}

char16_t alphabets::get_random_overall_char()
{
    int i = mr.get_rand_int_max(all_size);
    for (auto a : all_alphabets)
    {
        if (i < a.get_size())
        {
            return a.get_char(i);
        }
        i -= a.get_size();
    }
    return 0;
}

string alphabets::generate_valid_password()
{
    string s = "";
    while (s.size() < SIZEC)
    {
        s += (char)get_random_overall_char();
    }
    validate(s);
    return s;
}

void alphabets::validate(string s)
{
    bool f = true;
    while (f)
    {
        f = false;
        for (auto a : all_alphabets)
        {
            if (a.is_valid(s) != 0)
            {
                f = true;
                int v;
                while ((v = a.is_valid(s)) != 0)
                {
                    if (v < 0)
                    {
                        s[mr.get_rand_int_max(s.size())] = (char)a.get_random_char();
                    }
                    else
                    {
                        s[a.search_char(s)] = get_random_overall_char();
                    }
                }
            }
        }
    }
    cout << s;
    cout << endl;
}

int main()
{
    alphabets a = alphabets();
    cout << a.generate_valid_password();
    cout << endl;
    return 0;
}

Die Ausgabe ist:

lI4&5gP3bA)c(wcgm3kC40U.bL$57o
lI4&ugP3bA)c(wcg,3kCv0U.+L$)ao

Wie kommt das? In generate_valid_password() wird ein string angelegt. In validate(string s) wird dieser string "korrigiert".

Allerdings wird in validate(string s) nur auf einer Kopie von string s gearbeitet. Das heißt, der Funktionsaufruf von validate hat keine Auswirkungen an den Aufrufer generate_valid_password - oder anders: generate_valid_password gibt kein immer gültiges Passwort zurück.

---

Eine andere Frage: Ist denn for (auto a : all_alphabets) { wenigstens richtig - oder soll ich hier auch die Referenz verwenden: for (auto &a : all_alphabets) {?

Danke für eure Geduld mit mir...

[EinNutzer0]

So würde ich es tatsächlich als richtig(er) erachten:

#include <stdexcept>
#include <iostream>
#include <random>
#include <vector>
#include <algorithm>

#define SIZEC 200

using namespace std;

class mrand
{
private:
    mt19937 gen = mt19937(12); // CHANGE IT!!!

public:
    int get_rand_int_max(int max)
    {
        uniform_int_distribution<> distr(0, max - 1);
        return distr(gen);
    }
};

mrand mr = mrand();

class alphabet
{
private:
    vector<char16_t> all_chars;
    size_t min;
    size_t max;

public:
    alphabet(char16_t from, char16_t to, size_t min, size_t max);
    char16_t get_random_char();
    int search_char(string s);
    int is_valid(string s);
    int get_size();
    char16_t get_char(int i);
};

class alphabets
{
private:
    vector<alphabet> all_alphabets;
    size_t all_size = 0;

public:
    alphabets();
    char16_t get_random_overall_char();
    string generate_valid_password();
    void validate(string & s);
};

alphabet::alphabet(char16_t from, char16_t to, size_t min, size_t max)
{
    for (char16_t i = from; i <= to; i++)
    {
        all_chars.push_back(i);
    }
    this->min = min;
    this->max = max;
}

char16_t alphabet::get_random_char()
{
    return all_chars[mr.get_rand_int_max(all_chars.size())];
}

int alphabet::search_char(string s)
{
    int i;
    do
    {
        i = mr.get_rand_int_max(s.size());
    } while (find(all_chars.begin(), all_chars.end(), s[i]) == all_chars.end());
    return i;
}

int alphabet::is_valid(string s)
{
    int c = 0;
    for (auto i : s)
    {
        if (find(all_chars.begin(), all_chars.end(), i) != all_chars.end())
        {
            c++;
        }
    }
    return c < (int)min ? -1 : (c > (int)max ? 1 : 0);
}

int alphabet::get_size()
{
    return all_chars.size();
}

char16_t alphabet::get_char(int i)
{
    return all_chars[i];
}

alphabets::alphabets()
{
    all_alphabets.push_back(alphabet('a', 'z', 1, SIZEC));
    all_alphabets.push_back(alphabet('A', 'Z', 6, SIZEC));
    all_alphabets.push_back(alphabet('0', '9', 8, SIZEC));
    all_alphabets.push_back(alphabet('!', '/', 2, 5));
    for (auto & a : all_alphabets)
    {
        all_size += a.get_size();
    }
}

char16_t alphabets::get_random_overall_char()
{
    int i = mr.get_rand_int_max(all_size);
    for (auto & a : all_alphabets)
    {
        if (i < a.get_size())
        {
            return a.get_char(i);
        }
        i -= a.get_size();
    }
    return 0;
}

string alphabets::generate_valid_password()
{
    string s = "";
    while (s.size() < SIZEC)
    {
        s += (char)get_random_overall_char();
    }
    validate(s);
    return s;
}

void alphabets::validate(string & s)
{
    bool f = true;
    while (f)
    {
        f = false;
        for (auto & a : all_alphabets)
        {
            int v;
            while ((v = a.is_valid(s)) != 0)
            {
                f = true;
                if (v < 0)
                {
                    s[mr.get_rand_int_max(s.size())] = (char)a.get_random_char();
                }
                else
                {
                    s[a.search_char(s)] = get_random_overall_char();
                }
            }
        }
    }
    cout << s;
    cout << endl;
}

int main()
{
    alphabets a;
    cout << a.generate_valid_password();
    cout << endl;
    return 0;
}

Zu übersetzen mit: g++.

[wob]

Warum returnt dein is_valid eigentlich int statt bool und was testet es genau?

Warum nutzt du intern überall char16_t? Du castest das sowieso am Ende nach char, wenn du es an den string anhängst. Die Frage ist dann eher, wie man das hier Unicode-Kompatibel macht. Mit int32 (Code points) arbeiten oder gleich z.B. utf8 erzeugen?

Warum hat deine alphabet-Klasse eigentlich vector als Member, vo du doch eigentlich nur from und to bräuchtest - und was haben min und max im Alphabet verloren?

Was macht search_char, insbesondere warum ist da irgendwas mit random drin? Klingt erstmal so, als solle nach einem Zeichen gesucht werden, aber schon der String-Parameter verursacht gleich Fragezeichen bei mir.

Warum returnt get_size int? Warum heißt es nicht analog zur STL einfach nur size und returnt einen size_t?

Warum ist keine Klasse und keine Funktion dokumentiert? Ich bin ein Fan davon, dass alle Klassen und öffentlichen Funktionen zumindest einen kurzen Kommentar haben, wo steht, was die Funktion tut. Insbesondere wenn das nicht von Namen schon 100% klar ist.

Warum ist SIZEC ein Makro? Und wofür steht das? Ich finde den Namen nicht selbsterklärend.

Usw.

[Schlangenmensch]

@EinNutzer0 sagte in Bitte um Refactoring, Passwortgenerator mit Vorgaben:

Eine andere Frage: Ist denn for (auto a : all_alphabets) { wenigstens richtig - oder soll ich hier auch die Referenz verwenden: for (auto &a : all_alphabets) {?

for (auto a : all_alphabets) -> In jeder Iteration wird eine Kopie angelegt
for (auto& a : all_alphabets) -> In jeder Iteration wird eine Referenz verwendet

Eine Kopie kann teuer sein, bei einer Referenz kann man das Objekt verändern. Wenn man verhindern möchte, dass über eine Referenz das Objekt verändert wird, sollte man const verwenden.

for (const auto& a : all_alphabets)

Eigentlich sollte man immer const verwenden, wenn das möglich ist.

Es gäbe auch noch die rvalue Referenz

for (auto&& a : all_alphabets)

oder die Universal Referenz

template<typename T> 
void alphabets::validate(T&& s)

Zu erklären, wann man was verwendet und was man dabei beachten muss, dauert länger.

Prinzipiell würde deinem Code ein bisschen mehr const gut tun.

[hiyuck]

Hier sind einige Verbesserungen, die ich vorschlagen würde:

1.Verwende den using-Direktive nicht in der globalen Ebene. Stattdessen solltest du sie innerhalb einer Funktion oder eines Klassenkörpers verwenden.

2.Verwende constexpr statt #define für die SIZEC-Konstante.

3.Statt einer globalen Instanz von mrand, solltest du das random_device und mt19937-Objekte als private Member-Variablen in der mrand-Klasse haben. Dann könntest du einen Konstruktor schreiben, der diese Objekte initialisiert. Auf diese Weise könntest du auch mehrere Instanzen von mrand erstellen, falls dies benötigt wird.

4.Verwende const für Methoden, die den Zustand einer Klasse nicht ändern.

5.Verwende auto statt expliziter Typdeklarationen, wenn möglich.

6.Verwende std::u16string statt char16_t und std::u16string::value_type statt char16_t für den Typ der Elemente in all_chars.

7.Verwende std::array statt std::vector für all_chars, da es wahrscheinlich besser für die Leistung ist und die Größe bei der Compilierzeit festgelegt wird.

8.Füge override hinzu, wenn du Methoden von einer Basisklasse überschreibst.

9.Verwende std::vector::empty statt std::vector::size zum Prüfen, ob all_chars leer ist.

10.Verwende std::find statt std::vector::find zum Suchen von Elementen in all_chars, da es wahrscheinlich besser für die Leistung ist.

11.Verwende std::count_if statt einer Schleife und std::find zum Zählen der Anzahl von Elementen in s, die in all_chars enthalten sind.

12.Verwende std::uniform_int_distribution statt std::uniform_int_distribution<> und verwende std::mt19937::result_type statt int als Typ für die Verteilung.

[EinNutzer0]

Ein(!) neues Benutzerpasswort für den normalen Anwendungszweck zu generieren, ist doch überhaupt keine zeitkritische Angelegenheit... insofern kann ich die Punkte 6. bis 12. aus @hiyuck s Antwort _nicht_ unmittelbar nachvollziehen...

Dennoch Danke für die Reviews + Suggestions (und Ideas).

[SeppJ]

Punkte 6 und 12 haben nix mit Performance zu tun, sondern mit Korrektheit. Da überhaupt nicht ersichtlich ist, wie du überhaupt auf die Idee kommen könntest, dass dies mit Performance zu tun hätte, noch du deinen Gedankengang näher erläuterst, kann man dich auch nicht genauer korrigieren.

[EinNutzer0]

@hiyuck schreibt doch selber, dass die meisten Punkte auf Leistung, Performance und Mikrooptimierung beruhen... @SeppJ

Leider aber alles ohne Begründung dafür.

[Leon0402]

@EinNutzer0 Ich sehe jetzt nicht genau was du meinst. Bei Punkt 7 und Punkt 10 steht was von Leistung. Die beste Begründung ist das hier imo nicht. Aber sonst sehe ich bei 6 und 12 und schon mal gar nicht von 6 bis 12 nur Punkte über Performance.

[SeppJ]

@EinNutzer0 sagte in Bitte um Refactoring, Passwortgenerator mit Vorgaben:

@hiyuck schreibt doch selber, dass die meisten Punkte auf Leistung, Performance und Mikrooptimierung beruhen... @SeppJ

Leider aber alles ohne Begründung dafür.

Citation needed.

Bitte lern' Lesen! Wirklich. Es ist ganz schlimm. Egal ob Antworten auf Fragen hier im Forum oder Handbücher, deine Interpretationen sind stets komplett ausgedacht.