Password Generator fertig! Bitte um Kritik!

B1LLYTH3K1T

Hallo Freunde,
Ich habe gestern Abend meinen Password Generator fertig gestellt (wie schon in anderen Threads beschrieben, komme ich von Python).-
Der Generator Funktioniert soweit, nur leider stürtzt er bei meheren "Klicks" ab. (Vielleicht könnt ihr mir ja sagen wieso ).

Ich wäre euch sehr dankbar wenn ihr bisschen Kritik an meiner Programmierweise auslassen würdet, denn nur aus Fehlern kann man lernen. :face_with_stuck-out_tongue_closed_eyes:

#include "mainwindow.h"
#include "ui_mainwindow.h"
#include "qstring.h"
#include "QMessageBox"
#include "string"
#include "QVector"

#include <iostream>
#include <vector>

using namespace std;

MainWindow::MainWindow(QWidget *parent) :
    QMainWindow(parent),
    ui(new Ui::MainWindow)
{
    ui->setupUi(this);

    connect(ui->pushButton, &QPushButton::clicked, this, &MainWindow::generate_Key);

}




void MainWindow::generate_Key()
{
    QVector<QString> Keys;
    int number = ui->spinBox->value();

    QString generate_password = "";




    ui->lineEdit->clear();

    if (ui->checkBox_5->checkState() == 2)
    {
        QVector<QString> Key_Change;
        Key_Change << "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";
        for(int i = 0; i < Key_Change.size(); i++)
        {
            Keys.append(Key_Change[i]);
        }

    }
    if (ui->checkBox_5->checkState() == 2)
    {
        QVector<QString> Key_Change;
        Key_Change << "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";
        for(int i = 0; i < Key_Change.size(); i++)
        {
            Keys.append(Key_Change[i]);
        }
    }
    if (ui->checkBox_5->checkState() == 2)
    {
        QVector<QString> Key_Change;
        Key_Change << "1" << "2" << "3" << "4" << "5" << "6" << "7" << "8" << "9" << "0" << "1" << "2" << "3" << "4" << "5" << "6" << "7" << "8" << "9" << "0" << "1" << "2" << "3" << "4" << "5" << "6" << "7" << "8" << "9" << "0";
        for(int i = 0; i < Key_Change.size(); i++)
        {
            Keys.append(Key_Change[i]);
        }
    }
    if (ui->checkBox_5->checkState() == 2)
    {
        QVector<QString> Key_Change;
        Key_Change << "ä" << "ü" << "ö"<< "Ä" << "Ü" << "Ö" << "ä" << "ü" << "ö"<< "Ä" << "Ü" << "Ö"<< "ä" << "ü" << "ö" << "Ä" << "Ü" << "Ö" << "ä" << "ü" << "ö"<< "Ä" << "Ü" << "Ö";
        for(int i = 0; i < Key_Change.size(); i++)
        {
            Keys.append(Key_Change[i]);
        }
    }
    if (ui->checkBox_5->checkState() == 2)
    {
        QVector<QString> Key_Change;
        Key_Change << "!" << "§" << "$" << "%" << "&" << "/" << "(" << ")" << "=" << "?" << "!" << "§" << "$" << "%" << "&" << "/" << "(" << ")" << "=" << "?" << "$" << "%" << "&" << "/" << "(" << ")" << "=" << "?";
        for(int i = 0; i < Key_Change.size(); i++)
        {
            Keys.append(Key_Change[i]);
        }
    }
    if(ui->checkBox->checkState() == 0 && ui->checkBox_2->checkState() == 0 && ui->checkBox_3->checkState() == 0 && ui->checkBox_4->checkState() == 0 && ui->checkBox_5->checkState() == 0)
    {
        QMessageBox msgBox;
        msgBox.setText("Please Setup the Settings!");
        msgBox.exec();
    }

    for(int d = 0; d < number; d++)
    {
        generate_password = generate_password + Keys.at(rand() % Keys.size() + 1);
        ui->lineEdit->setText(generate_password);
    }


}




MainWindow::~MainWindow()
{
    delete ui;
}

Mechanics

Es ist zu wenig Code, um zu sagen, es wäre scheiße. Für ein in 10 Minuten hingerotztes Testprojekt, das genau das tut was es soll und dann gleich weggeworfen wird, wärs vielleicht ok.
Aber wenn du programmieren lernen willst, ist sowas wie
if (ui->checkBox_5->checkState() == 2)
absolut unwartbar und sinnlos. Fang das gar nicht erst an.
Außerdem sind diese lokalen Stringlisten, die du irgendwo reinkopierst auch keine gute Idee. Das hab ich dir auch schon in dem anderen Thread gesagt. Überleg dir irgendeine andere Datenstruktur. Da ist kein Platz für "if else".
Nimm dir Maps, oder andere Datenstrukturen, initialisiere das "irgendwie" am Anfang. Und mit irgendwie mein ich so, dass gleich klar ist, welchem Zweck das dient. z.B. wenn das eine Option für Zahlen ist, dann soll das im Code ersichtlich sein. Und mach dann alle Eventhandler generisch, sodass die einfach in eine Datenstruktur durchgreifen und sich die Objekte holen, die für die Option benötigt wird. Kein Code im if-else, kein Mensch will über sowas nachdenken oder das warten.
Das wäre übrigens auch im Python genauso.

B1LLYTH3K1T

@mechanics Gut danke Kennst du zufällig eine Deutsche Seite über QT wo man bisschen rumschöckern kann ? Ich hab mir C++ bisschen angeeignet aber ich finde QT ist da etwas anders. ABER TOP GUI!

Swordfish

@b1llyth3k1t sagte in Password Generator fertig! Bitte um Kritik!:

Ich hab mir C++ bisschen angeeignet

Deinem Code nach hast du keinen Tau von C++.

Mechanics

Ich hab jetzt kein Problem mit deinem Qt Code... Und nein, es gibt keine guten (deutschen) Seiten. Im Grunde gibts bei Qt nicht viel zu wissen. Es gibt gewisse Grundlagen, die man kennen muss, wie QObject, Signals/Slots usw... Viel ist es ja nicht. Und der Rest ist einfach nur die Referenz. Und da schaut man einfach in die offizielle Doku, die ist ganz gut. Was es dann vielleicht noch gibt, sind irgendwelche "fortgeschrittenen" Probleme. Aber das ist nichts zum Lernen/Beschreiben, wenn man mal ein konkretes Problem hat und nicht weiß, wie man das am besten löst, versucht man das zu googeln und findet öfter mal Forenbeiträge oder ähnliches, wo das schon mal jemand gelöst hat. Aber sehr wahrscheinlich nicht auf Deutsch.

Ich hab jetzt auch nicht so wirklich ein Problem mit deinem C++ Code... Dafür ist es zu wenig, und wenn da was nicht optimal ist, ist mir das jetzt gar nicht so aufgefallen, bzw. war mir dann schon egal.

Dein Hauptproblem ist aus meiner Sicht, dass du einfach viel zu wenig Ahnung von Softwaredesign hast. Man schreibt keine Software, wie du das gemacht hast. Das kannst du im Endeffekt auch iterativ lösen, glaube ich. Schau dir den ganzen "komischen" Code an und versuch das nach und nach aufzulösen. Keine Redundanzen. Kein if-else...

hustbaer

@b1llyth3k1t sagte in Password Generator fertig! Bitte um Kritik!:

    QVector<QString> Key_Change;
    Key_Change << "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";
    for(int i = 0; i < Key_Change.size(); i++)
    {
        Keys.append(Key_Change[i]);
    }

Verstehe denn Sinn davon nicht.
Wieso nicht gleich

         Keys << "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";

Und das...

    if(ui->checkBox->checkState() == 0 && ui->checkBox_2->checkState() == 0 && ui->checkBox_3->checkState() == 0 && ui->checkBox_4->checkState() == 0 && ui->checkBox_5->checkState() == 0)
    {
        QMessageBox msgBox;
        msgBox.setText("Please Setup the Settings!");
        msgBox.exec();
    }

Also erstmal würde ich das eher so schreiben:

    if(Keys.empty())
    {
        QMessageBox msgBox;
        msgBox.setText("Please Setup the Settings!");
        msgBox.exec();
    }

Und dann fehlt da noch ein return; wenn du nicht willst dass dir der Modulo-Operator (%) in der folgenden Schleife um die Ohren fliegt.

Und natürlich

    for(int d = 0; d < number; d++)
    {
        generate_password = generate_password + Keys.at(rand() % Keys.size() + 1);
        ui->lineEdit->setText(generate_password);
    }

Wozu die UI bei jedem Schleifendurchlauf updaten? Und wieso die umständliche "a = a + b" Schreibweise?
=>

    for(int d = 0; d < number; d++)
        generate_password += Keys.at(rand() % Keys.size() + 1);

    ui->lineEdit->setText(generate_password);

Und ... + 1? Huch? Willst du dass dir ein out_of_range rausfliegt?

hustbaer

Dann hätten wir den Code mal mehr oder weniger "sauber". DANN könnten wir uns die ganzen Variablen-Namen ansehen. number eieiei. Oder generate_password ... das sollte wohl eher generated_password heissen.

B1LLYTH3K1T

@hustbaer Vielen Dank ! Das hat mir echt super weiter geholfen !

B1LLYTH3K1T

@mechanics Du hast von Software design geredet irgendwelche guten Bücher dazu ?

B1LLYTH3K1T

@hustbaer sagte in Password Generator fertig! Bitte um Kritik!:

Also erstmal würde ich das eher so schreiben:
if(Keys.empty())
{
QMessageBox msgBox;
msgBox.setText("Please Setup the Settings!");
msgBox.exec();
}

Und dann fehlt da noch ein return; wenn du nicht willst dass dir der Modulo-Operator (%) in der folgenden Schleife um die Ohren fliegt.
Und natürlich
for(int d = 0; d < number; d++)
{
generate_password = generate_password + Keys.at(rand() % Keys.size() + 1);
ui->lineEdit->setText(generate_password);
}

Wozu die UI bei jedem Schleifendurchlauf updaten? Und wieso die umständliche "a = a + b" Schreibweise?
=>
for(int d = 0; d < number; d++)
generate_password += Keys.at(rand() % Keys.size() + 1);
ui->lineEdit->setText(generate_password);
Und ... + 1? Huch? Willst du dass dir ein out_of_range rausfliegt?

Kannst du mir das nochmal genau erläutern?

Mechanics

@b1llyth3k1t sagte in Password Generator fertig! Bitte um Kritik!:

@mechanics Du hast von Software design geredet irgendwelche guten Bücher dazu ?

Auswending nicht, aber da gibts einiges, was man lesen könnte.
Ein bisschen was über Clean Code, C++ Core Guidelines, gibt sicher auch lauter Bücher über allgemeine Prinzipien.
Dann sollte man sich auf jeden Fall mal die Design Patterns anschauen.
Über Software Architektur (weiterführende Patterns, größere Zusammenhänge) gibts auch paar Bücher, ist aber schon Jahre her, dass ich die gelesen habe.

Schau dir doch einfach mal in der Bibliothek einer FH oder Uni bei dir in der Nähe um. Zumindest bei uns in der FH gibts sehr viele Bücher (auch zu diesen Themen), die kann man in Ruhe mal durcharbeiten. Gibt auch sehr viele sehr ähnliche Bücher, irgendwann kann man das gut unterscheiden und dann weiß man, dass 10-20 Bücher wahrscheinlich reichen. Bei uns kann man auch Bücher bestellen lassen, wenn die Bibliothek sie noch nicht hat. Aber für den Anfang reichen wahrscheinlich die Bücher, die schon da sind, und später "stolperst" du dann selber über gute Bücher, die man lesen sollte.

hustbaer

@b1llyth3k1t sagte in Password Generator fertig! Bitte um Kritik!:

Kannst du mir das nochmal genau erläutern?

Was genau verstehst du nicht?

Mechanics

Wahrscheinlich versteht er nicht, dass die Indizes bei 0 anfangen?

hustbaer

@Mechanics
Naja er hat da ziemlich viel zitiert und ich hab nicht vor alles da auf einem Niveau zu erklären dass jemand der überhaupt nichts davon versteht es dann versteht. Weil mir das zu viel Aufwand ist.