w724 Router login. Meinungen zum Code

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
lexx
User
Beiträge: 9
Registriert: Sonntag 23. Dezember 2018, 14:15

Freitag 17. Mai 2019, 10:05

Hallo zusammen,

ich bin ein Anfänger in Sachen Programmieren und habe mir versucht einiges anzueigenen.
Jetzt würde ich gerne mal von Experten wissen, ob man den Code so lassen kann oder ob er Scheiße ist :D

Code: Alles auswählen

import requests,hashlib,re,datetime
class Router:
    def __init__(self):
        self.headers= ""
        self.httoken=""
        self.session=""
    def GetConnection(self):
        self.session = requests.Session()
        self.session.post('http://192.168.2.1/data/Login.json', data={'password': hashlib.md5(bytes("dasSicherstePasswort" , 'utf-8')).hexdigest(), 'showpw': '0', 'httoken': ''})
        self.httoken = re.findall('_httoken = (\d*);', self.session.get('http://192.168.2.1' + '/html/content/overview/index.html?lang=de').text)[0]
        self.headers = {'Referer': 'http://192.168.2.1' + '/html/content/overview/index.html?lang=de'}
        #return self.headers,self.httoken,self.session
    def verbinungsTest(self):
        if self.headers:
            return self.headers,self.httoken,self.session
            print("geht")
        else:
            print("geht nicht")
            self.GetConnection()
            return self.headers,self.httoken,self.session
    def getTelefonbuch(self):
        headers,httoken,session = self.verbinungsTest()
        return session.get("http://192.168.2.1/data/PhoneBook.json",params={"_tn": httoken},headers=headers)
    def Wlanoff(self):
        headers,httoken,session = self.verbinungsTest()
        return session.post("http://192.168.2.1/data/Modules.json",data={"use_wlan": "0", 'httoken': httoken}, headers=headers)
    def Wlanon(self):
        headers,httoken,session = self.verbinungsTest()
        return session.post("http://192.168.2.1/data/Modules.json",data={"use_wlan": "1", 'httoken': httoken}, headers=headers)
    def SetWlan5ghz(self):
        headers,httoken,session =self.verbinungsTest()
        data = {"wlan_5ghz_ssid" : "Wlan von mir", "httoken": httoken}
        return session.post("http://192.168.2.1/data/WLANBasic.json",data=data,headers=headers)
    def ghzOn(self):
        headers,httoken,session =self.verbinungsTest()
        data = {"use_wlan_5ghz" : "1", "httoken" : httoken}
        response = session.post("http://192.168.2.1/data/Modules.json",data=data, headers=headers)
        print(response.text)
    def ghzOff(self):
        headers,httoken,session =self.verbinungsTest()
        data = {"use_wlan_5ghz" : "0", "httoken" : httoken}
        response = session.post("http://192.168.2.1/data/Modules.json",data=data, headers=headers)
        #session.close()
        print(response.text)
    def getRuflisten(self,typ="all"):
        headers, httoken, session = self.verbinungsTest()
        data = session.get('http://192.168.2.1/data/PhoneCalls.json', params={"_tn": httoken}, headers=headers).json()
        if typ == 'anrufe':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'adddialedcalls']
        elif typ == 'eingehend':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'addtakencalls']
        elif typ == 'verpasst':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'addmissedcalls']
        elif typ == 'all':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:]]
        elif typ == 'heute':
            return [i for i in [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:]] if
                    datetime.date.today().strftime("%d.%m.%Y") in i[1]]
        #return data
#test = Router()
#print(test.getRuflisten())
__deets__
User
Beiträge: 6059
Registriert: Mittwoch 14. Oktober 2015, 14:29

Freitag 17. Mai 2019, 11:18

Tendenziell eher nicht so gut.

- Mit + zusammengestückelte strings - macht man nicht, benutz format.
- permanent wiederholte Daten wie die HTTP URL gehören in eine Variable. Nicht jedes Mal hingeschrieben.
- prints() haben darin nichts verloren. Ergebnisse gehören per Return zurückgegeben.
- du verbindest dich. Du testest nicht die Verbindung. Warum heißt das verbindungsTest?
- die Namen sind nicht der Konvention entsprechend,, die Leerzeichen um Operatoren inkonsequent, zwischen den Methoden keine leerzeile.
- das Passwort ist kein Parameter, sondern hart kodiert.

Das nur auf den ersten Blick.
lexx
User
Beiträge: 9
Registriert: Sonntag 23. Dezember 2018, 14:15

Freitag 17. Mai 2019, 13:51

Erst mal Dankschön für die schnelle Antwort :cry:

Könnte alle Daten natürlich in einem rutsch holen, was ich aber nicht wollte, darum der Verbindungstest.
Wenn ich das richtig sehe, prüfe ich ob self.headers wahr ist, wenn dem so ist holt der direkt die Daten, wenn nicht verbindet er erst einmal.
Habe den Code noch mal überarbeitet

Code: Alles auswählen

class Router:
    def __init__(self):
        self.headers = ""
        self.httoken = ""
        self.session = ""
        self.url_router = "http://192.168.2.1{}"
        self.password =  hashlib.md5(bytes("Mb2.r5oHf-0t" , 'utf-8')).hexdigest() #https://www.der-postillon.com/2014/04/it-experten-kuren-mb2r5ohf-0t-zum.html

    def get_connection(self):
        self.session = requests.Session()
        self.session.post(self.url_router.format("/data/Login.json"), data={'password': self.password, 'showpw': '0', 'httoken': ''})
        self.httoken = re.findall('_httoken = (\d*);', self.session.get(self.url_router.format('/html/content/overview/index.html?lang=de')).text)[0]
        self.headers = {'Referer': self.url_router.format('/html/content/overview/index.html?lang=de')}

    def verbindungstest(self):
        if self.headers:
            #print("geht")


        else:
            #print("geht nicht")
            self.get_connection()


    def get_telefonbuch(self):
        self.verbindungstest()
        return self.session.get(self.url_router.format("/data/PhoneBook.json"),params = {"_tn": self.httoken},headers=self.headers)

    def wlan_off(self):
        self.verbindungstest()
        return self.session.post(self.url_router.format("/data/Modules.json"),data = {"use_wlan": "0", 'httoken': self.httoken}, headers=self.headers)

    def wlan_on(self):
        self.verbindungstest()
        return self.session.post(self.url_router.format("/data/Modules.json"),data={"use_wlan": "1", 'httoken': self.httoken}, headers = self.headers)

    def set_wlan_5ghz(self):
        self.verbindungstest()
        data = {"wlan_5ghz_ssid" : "Wlan von mir", "httoken": self.httoken}
        return self.session.post(self.url_router.format("/data/WLANBasic.json"),data = data,headers = self.headers)

    def ghz_on(self):
        self.verbindungstest()
        data = {"use_wlan_5ghz" : "1", "httoken" : self.httoken}
        response = self.session.post(self.url_router.format("data/Modules.json"),data=data, headers = self.headers)
        return response.text

    def ghz_off(self):
        self.verbindungstest()
        data = {"use_wlan_5ghz" : "0", "httoken" : self.httoken}
        response = self.session.post(self.url_router.format("/data/Modules.json"),data = data, headers = self.headers)
        #session.close()
        return response.text

    def get_ruflisten(self,type="all"):
        self.verbindungstest()
        data = self.session.get(self.url_router.format("/data/PhoneCalls.json"), params={"_tn": self.httoken},headers=self.headers).json()
        if type == 'anrufe':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'adddialedcalls']
        elif type == 'eingehend':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'addtakencalls']
        elif type == 'verpasst':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'addmissedcalls']
        elif type == 'all':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:]]
        elif type == 'heute':
            return [i for i in [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:]] if
                    datetime.date.today().strftime("%d.%m.%Y") in i[1]]
__deets__
User
Beiträge: 6059
Registriert: Mittwoch 14. Oktober 2015, 14:29

Freitag 17. Mai 2019, 14:33

Das Passwort ist immer noch Hard kodiert. Statt als Argument übergebenen zu werden. Ebenso wie die url. Damit hast du und unfreiwillig dein Passwort verraten. Und natürlich ist die Klasse nicht universell nutzbar.

Und es ist immer noch kein Test. Es ist der Aufbau der Verbindung.
Benutzeravatar
__blackjack__
User
Beiträge: 3883
Registriert: Samstag 2. Juni 2018, 10:21
Wohnort: 127.0.0.1
Kontaktdaten:

Freitag 17. Mai 2019, 14:44

@lexx: Warum wolltest Du das nicht? Und wenn dann wäre das IMHO besser mit einem `property()` gelöst, und zwar genau mit einem, das heisst man müsste die Attribute die da in `get_connection()` gesetzt werden noch einmal in einem eigenen Typ kapseln. Aber das ist alles unnötig umständlich.

Die Initialisierung mit leeren Zeichenketten ist falsch wenn da hinterher andere Typen dran gebunden werden. Für „nichts“ gibt es den Wert `None`.

Der Name `get_connection()` ist irreführend, weil man da als Leser erwartet von dieser Methode eine Verbindung als Rückgabewert zu bekommen. Sollte die Methode öffentlich sein? Falls ja sollte sie auch sauber damit umgehen können wenn sie mehr als einmal aufgerufen wird.

Zu `verbindungstest()` wurde ja schon mal gesagt, dass da keine Verbindung getestet wird. Und die sollte wirklich nicht zur öffentlichen API gehören.

Mir fehlt da irgendwie eine `close()`-Methode. Und dann könnte man aus der Klasse auch gleich einen Kontextmanager machen (``with``, `__enter__()`, `__exit__()`).

Die Methoden haben immer noch sehr viel gemeinsam das man da heraus ziehen könnte.

Es ist auch IMHO ein bisschen komisch das der Benutzer sich mit `requests.Response`-Objekten beschäftigen muss, das interessiert ihn doch eigentlich gar nicht wenn er die Daten haben möchte. Sogar selbst auf den Status der `Response` prüfen muss der Aufrufer.

Bei den Ruflisten ist es verwirrend das in verschachtelten „list comprehensions“ die Name `i` und `data` mehrfach verwendet wird. `i` ist da auch ein sehr schlechter Name solange er nicht an eine ganze Zahl gebunden wird. Du solltest da für die Laufvariablen Namen verwenden die Beschreiben was der Wert bedeutet, und jeden Namen innerhalb eines Ausdrucks nur einmal für eine Sache verwenden.

Bei den Ruflisten würde ich keine Zeichenkette als Typ verwenden sondern einen `enum.Enum`-Typ erstellen. Am besten ein `enum.Flag` und den Code so schreiben das *einmal* gefiltert wird und zwar auch auf mehr als einen Typ. Dann kann der Aufrufer eine beliebige Kombination von Typen abfragen.
“There's also a certain pleasure in actually getting things to work in Java, somewhat like the pleasure, I imagine, of building ships in bottles.”
— David Cook in c.l.p
lexx
User
Beiträge: 9
Registriert: Sonntag 23. Dezember 2018, 14:15

Freitag 17. Mai 2019, 17:54

Hallo __blackjack__,
vielen Dank erst mal für deine freundliche Herangehensweise :D
Wie gesagt ich bin kein Profi und noch in der Lernphase
Manche Sachen die ihr so als Antworten schreibt, muss ich erst mal nachschlagen damit ich es verstehe. Aber ich werde alles versuchen um es umzusetzen.
__blackjack__ hat geschrieben:
Freitag 17. Mai 2019, 14:44
@lexx: Warum wolltest Du das nicht? Und wenn dann wäre das IMHO besser mit einem `property()` gelöst, und zwar genau mit einem, das heisst man müsste die Attribute die da in `get_connection()` gesetzt werden noch einmal in einem eigenen Typ kapseln. Aber das ist alles unnötig umständlich.
Ursprünglich habe ich Daten die mal holen konnte alle in einem Rutsch geholt, das hat mir dann aber zu lange gedauert und dann habe ich mich dazu entschieden, nur die Datem zu holen die ich brauche.
Ob das jetzt der professionelle Weg ist weiß nicht, hat aber soweit geklappt.

.
__blackjack__ hat geschrieben:
Freitag 17. Mai 2019, 14:44
Die Initialisierung mit leeren Zeichenketten ist falsch wenn da hinterher andere Typen dran gebunden werden. Für „nichts“ gibt es den Wert `None`.
Ist erledigt und macht auch vollkommen Sinn. Hoffentlich habe ich das auch richtig umgesetzt.
__blackjack__ hat geschrieben:
Freitag 17. Mai 2019, 14:44
1. Mir fehlt da irgendwie eine `close()`-Methode. Und dann könnte man aus der Klasse auch gleich einen Kontextmanager machen (``with``, `__enter__()`, `__exit__()`).

2. Es ist auch IMHO ein bisschen komisch das der Benutzer sich mit `requests.Response`-Objekten beschäftigen muss, das interessiert ihn doch eigentlich gar nicht wenn er die Daten haben möchte. Sogar selbst auf den Status der `Response` prüfen muss der Aufrufer.

3.Bei den Ruflisten würde ich keine Zeichenkette als Typ verwenden sondern einen `enum.Enum`-Typ erstellen. Am besten ein `enum.Flag` und den Code so schreiben das *einmal* gefiltert wird und zwar auch auf mehr als einen Typ. Dann kann der Aufrufer eine beliebige Kombination von Typen abfragen.
Punkt 1 und 3 verstehe ich leider noch nicht so ganz.
Zu punkt 2 sage ich jetzt mal, dass ich noch daran arbeite, nur der Weg ist mir selber noch nicht ganz klar :roll:

Hier dann eventuell was besser ?

Code: Alles auswählen

import requests,hashlib,re,datetime
class Router:
    def __init__(self,passwort):

        self.url_router = "http://192.168.2.1{}"
        self.index = '/html/content/overview/index.html?lang=de'
        self.password =  hashlib.md5(bytes(passwort , 'utf-8')).hexdigest() #https://www.der-postillon.com/2014/04/it-experten-kuren-mb2r5ohf-0t-zum.html
        self.login()
    def login(self):
            self.session = requests.Session()
            self.session.post(self.url_router.format("/data/Login.json"), data={'password': self.password, 'showpw': '0', 'httoken': ''})
            self.httoken = re.findall('_httoken = (\d*);', self.session.get(self.url_router.format(self.index)).text)[0]
            self.headers = {'Referer': self.url_router.format(self.index)}



    def get_telefonbuch(self):
        return self.session.get(self.url_router.format("/data/PhoneBook.json"),params = {"_tn": self.httoken},headers=self.headers).json()

    def wlan_off(self):
        return self.session.post(self.url_router.format("/data/Modules.json"),data = {"use_wlan": "0", 'httoken': self.httoken}, headers=self.headers).json()

    def wlan_on(self):
        return self.session.post(self.url_router.format("/data/Modules.json"),data={"use_wlan": "1", 'httoken': self.httoken}, headers = self.headers).json()

    def set_wlan_5ghz(self):
        data = {"wlan_5ghz_ssid" : "Wlan von mir", "httoken": self.httoken}
        return self.session.post(self.url_router.format("/data/WLANBasic.json"),data = data,headers = self.headers).json()

    def fuenf_ghz_on(self):
        data = {"use_wlan_5ghz" : "1", "httoken" : self.httoken}
        response = self.session.post(self.url_router.format("data/Modules.json"),data=data, headers = self.headers)
        return response.text

    def fuenf_ghz_off(self):
        data = {"use_wlan_5ghz" : "0", "httoken" : self.httoken}
        response = self.session.post(self.url_router.format("/data/Modules.json"),data = data, headers = self.headers)
        return response.text

    def get_ruflisten(self,type="all"):

        data = self.session.get(self.url_router.format("/data/PhoneCalls.json"), params={"_tn": self.httoken},headers=self.headers).json()
        if type == 'anrufe':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'adddialedcalls']
        elif type == 'eingehend':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'addtakencalls']
        elif type == 'verpasst':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:] if
                    i["varid"] == 'addmissedcalls']
        elif type == 'all':
            return [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:]]
        elif type == 'heute':
            return [i for i in [(i["varid"], [data["varvalue"] for data in i["varvalue"]]) for i in data[19:]] if
                    datetime.date.today().strftime("%d.%m.%Y") in i[1]]
Antworten