3. Bitte um Refactoring, Passwortgenerator mit Vorgaben

Bitte um Refactoring, Passwortgenerator mit Vorgaben

  • E
    Gesperrt

    @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...

    0
    S
     1 Antwort
  • E
    Gesperrt

    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++.

    0
  • W

    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.

    0
  • S

    @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.

    0
  • H

    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.

    0
    E
     1 Antwort
  • E
    Gesperrt

    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).

    0
  • Mod

    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.

    0
  • E
    Gesperrt

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

    Leider aber alles ohne Begründung dafür.

    0
    L
     2 Antworten
  • L

    @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.

    0
  • Mod

    @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.

    0
  • E
    Gesperrt

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

    wahrscheinlich besser für die Leistung ist

    Tipp: Strg+F im Browser und "Leistung" eingeben

    0
  • N
    Gesperrt

    Dieser Beitrag wurde gelöscht!
    0
  • Mod

    Wenn du darauf bestehst, machen wir aus einem temporären Bann, eben einen Permabann.

    0
Anmelden zum Antworten