Das deutsche Python-Forum

Sehr geehrte python-forum Community,
Ich bin noch recht neu in Python und dies ist eines meiner ersten Projekte. Da ich schon seit einiger Zeit in diesem Forum unterwegs bin, weiß ich, dass ihr konstruktive Kritik (genau das was ein Anfänger braucht[vor allem die Tipps hier waren ziemlich hilfreich

])gebt. Deswegen bitte ich euch mir eventuell einige Tipps, beispielsweise zu schöneren "Ausdrucksarten". Danke, dass ihr diesen Post durchlest,

newchild

Code: Alles auswählen

class code(object):
    def __init__(self,list=[],real_list=[]):
        self.list=list
        self.real_list=real_list



    def stringtofloat(self,string):
        if string.lower()=='a':
            self.l=1*self.key
        if string.lower()=='b':
            self.l=2*self.key
        if string.lower()=='c':
            self.l=3*self.key
        if string.lower()=='d':
            self.l=4*self.key
        if string.lower()=='e':
            self.l=5*self.key
        if string.lower()=='f':
            self.l=6*self.key
        if string.lower()=='g':
            self.l=7*self.key
        if string.lower()=='h':
            self.l=8*self.key
        if string.lower()=='i':
            self.l=9*self.key
        if string.lower()=='j':
            self.l=10*self.key
        if string.lower()=='k':
            self.l=11*self.key
        if string.lower()=='l':
           self.l=12*self.key
        if string.lower()=='m':
            self.l=13*self.key
        if string.lower()=='n':
            self.l=14*self.key
        if string.lower()=='o':
            self.l=15*self.key
        if string.lower()=='p':
            self.l=16*self.key
        if string.lower()=='q':
            self.l=17*self.key
        if string.lower()=='r':
           self.l=18*self.key
        if string.lower()=='s':
            self.l=19*self.key
        if string.lower()=='t':
            self.l=20*self.key
        if string.lower()=='u':
            self.l=21*self.key
        if string.lower()=='v':
            self.l=22*self.key
        if string.lower()=='w':
            self.l=23*self.key
        if string.lower()=='x':
            self.l=24*self.key
        if string.lower()=='y':
            self.l=25*self.key
        if string.lower()=='z':
            self.l=26*self.key
        self.list+=[self.l]
    def encode(self):
        text=input('text')
        self.key=int(input('key(numbers only)'))
        for i in range(len(text)):
            self.stringtofloat(text[i])
            helpervar=self.list[i]
            self.real_list+=[helpervar]
        print(self.list)


x=code()
x.encode()

Hallo und willkommen im Forum!

Ich arbeit die Anmerkungen mal von oben nach unten ab:

- Die code-Klasse ist überflüssig, Funktionen sind hier besser angebracht
- Schau dir mal PEP 8 an, Klassen werden in Python in CamelCase geschrieben, damit kann man sich leichter von anderen Namen unterscheiden.
- Die default-Parameter in der __init__-Methode machen sicher nicht das, was du erwartest. Die default-Werte werden genau einmal, nämlich bei der Erzeugung der Methode, berechnet. Das heißt, alle code-Instanzen teilen sich die selbe Liste. Üblicherweise wird das so gelöst:

Code: Alles auswählen

def __init__(self, spam=None):
    if spam is None:
        spam = []

- "list" und "real_list" sind ganz schlechte Namen. Zum einen steckt da der Datentyp im Namen drin drin (was keine gute Idee ist, da sich dieser häufig mal ändert und dann Falschinformation im Code steht) und, was viel schlimmer ist, der Name sagt nichts aus. Verwende aussagekräftige Namen die beschreiben, was an den Namen gebunden ist. Weiter verdeckt "list" den Namen des Listendatentyps; keine gute Idee!
- "stringtofloat" sollte besser "string_to_float" heißen oder aber besser ganz anders. Hier wird nirgends ein String in einen Float gewandelt. Eigentlich wird ein Zeichen in einen Integer verwandelt.
- "string" ist wieder ein schlechter Name. Er sagt genau nichts aus und verdeckt auch noch den Namen eines Moduls.
- Statt überall ``string.lower()``-Aufrufe zu machen, kannst du den auch einmal am Anfang machen:

Code: Alles auswählen

string = string.lower()
if string == "a":
    ...

- Wenn du so eine Reihe an if-Statements hast, welche sich gegenseitig ausschließen, dann solltest du elif verwenden:

Code: Alles auswählen

if if string.lower()=='a':
    self.l=1*self.key
elif string.lower()=='b':
    ....

- Das Ergebnis von ``stringtofloat`` solltest du nicht an "self.l" binden, sondern einfach mittels return zurückgeben. Und schon gar nicht sollte ``self.list`` am Ende so seltsam geändert werden.
- Du solltest dir Gedanken darüber machen, was bei anderen Zeichen, zum Beispiel "1", "2" oder "3", im zu verschlüsselnden String passiert. Teste es einfach mal.
- Schau dir die ord-Funktion an.
-Kopiere keinen Code, so funktioniert Programmieren nicht. Du hast ein ganz klares Muster bei der Verschlüsselung, das musst du in Code umsetzen. Zum Beispiel so:

Code: Alles auswählen

letter = letter.lower()
for index, s in enumerate(string.ascii_lowercase:)
    if s == letter:
        return (index + 1) * key

Oder noch kompakter:

Code: Alles auswählen

return (dict(enumerate(string.ascii_lowercase))[letter.lower()] + 1) * key

- Trenne Berechnung und Ein- und Ausgabe. Die Eingabe des Texts hat in der encode-Methode nichts zu suchen.
- ``for i in range(len(text))`` ist ein Antipattern in Python. Du kannst direkt über die Elemente einer Liste iterieren: ``for element in text``. Wenn du doch mal den Index brauchst, dann verwende die enumerate-Funktion
- Die ganze Schleife ist total sinnlos. Erst packst du das verschlüsselte Element in "self.list", dann nimmst du das letzte Element aus "self.list" und hängst das an "self.real_list" an. Wenn du das Ergebnis gleich zurückgibst, dann musst du so einen Unsinn nicht mahchen.
- "self.l" ist überflüssig.
- Elemente werden mit append an eine Liste angehängt und nicht mittels +=
- Code sollte nie auf Modulebene stehen, packe das nach dem entsprechendem Muster in eine main-Funktion. Dann wird der Code auch nur ausgeführt, wenn du die Datei direkt ausführst. Damit kannst du das Modul auch importieren und an anderer stell nutzen:

Code: Alles auswählen

def main():
    x = code()
    x.encode()

if __name__ == "__main__":
    main()

Das ist jetzt natürlich eine gaze Menge, arbeite dich da mal durch.

@newchild: von oben nach unten: »code« hält sich nicht an die Namenskonvention, die sagt, dass Klassen mit einem Großbuchstaben anfangen. Veränderbare Objekte wie Listen sollten nicht als Defaultattribute gesetzt werden; probier mal aus, eine zweite code-Instanz zu erstellen. Alle Attribute sollten einmal in »__init__« gesetzt werden, bei Dir fehlen »l«, »key«. Wenn nur eine der »if«-Abfragen erfüllt sein kann, solltest Du »elif« verwenden. Dazu sind die ganzen »if«s zu umständlich, über »ord« läßt sich einfach ein Buchstabe in ihren ASCII-Wert umwandeln. Was passiert, wenn string kein Buchstabe ist? Die Funktion »stringtofloat« macht auch nicht das, was ihr Name sagt. Es wird kein »float« zurückgegeben, sondern das Attribut »l« gesetzt und noch die Liste »list« erweitert. Eine Funktion sollte genau eine Sache tun und am besten die, die der Name andeutet. »l« ist übrigens ein schlechter Variablenname, er ist nicht nur zu kurz, nichtssagen, sondern kann noch leicht mit I oder 1 verwechselt werden. »list« ist übrigens genauso nichtssagend. Warum gibt es eigentlich noch eine »real_list«? Was ist an der realer? Listen erweitert man »append«. »encode« macht die ganze Ein- und Ausgabe. Das sollte aber nie mit wirklicher Berechnung vermischt werden, was der Name der Funktion wieder andeutet. Bei »encode« erwarte ich nicht, dass der Benutzer etwas eingeben muß. »for i in range...« ist ein Antipattern, Du solltest direkt über die Zeichen in »text« iterieren. Woher kommt diese magische Liste »list«. Das wird an der Stelle nicht deutlich. »real_list« wird auch nie verwendet.

Danke für die Verbesserungsvorschläge

Ich denke jetzt bin ich erstmal beschäftigt ^^`
Wenn ich den Code überarbeitet habe, kann ich ihn ja nochmal zeigen und (hoffentlich) wieder daraus lernen.
Nochmals Danke an dieser Stelle

newchild hat geschrieben:

Code: Alles auswählen

        [...]
        if string.lower()=='a':
            self.l=1*self.key
        if string.lower()=='b':
            self.l=2*self.key
        if string.lower()=='c':
            self.l=3*self.key
        if string.lower()=='d':
            self.l=4*self.key
        if string.lower()=='e':
            self.l=5*self.key
        if string.lower()=='f':
            self.l=6*self.key
        [...]
        if string.lower()=='y':
            self.l=25*self.key
        if string.lower()=='z':
            self.l=26*self.key
        [...]

Es sollte einem eigentlich auch als Anfänger auffallen, dass hier eine gewisse Systematik vorliegt. Hier liegt sogar eine äußerst simple Systematik vor. Wenn value der String ist, dann ist der gesuchte Wert der für die Multiplikation benutzt wird ord(value)-96.

Bitte mache dir das nächste mal wenn du im Code offensichtliche Dopplungen mit nur kleinen Abweichungen findest Gedanken darüber, wie man den Code vereinfachen kann. Das kann durch eine Formal sein wie hier oder auch durch Auslagern der doppelten Codepassagen in eine Funktion.

Das deutsche Python-Forum

Verschlüsselungsscript

Verschlüsselungsscript

Re: Verschlüsselungsscript

Re: Verschlüsselungsscript

Re: Verschlüsselungsscript

Re: Verschlüsselungsscript