Mini-Uhr

[sisu]

Hallo,

ich bin noch ziemlich neu bei Python und versuche mich mit Code-Spielereien ein wenig zu üben ^^
Habe eine kleine Uhr geschrieben, die zugegebenermaßen bis vor einer halben Stunde noch gar keine richtige Uhr war: Man konnte die aktuelle Zeit nur selbst eingeben, weil ich nicht wusste, wo man die Systemzeit herbekommt. Dann aber rettete mich dieser Post hier. Coole Sache, so schnell die Lösung zu einem Problem zu finden!

Nun ist es also tatsächlich eine Uhr, kann aber auch als Stoppuhr verwendet werden, wenn Stunde, Minute und Sekunde auf Null gesetzt werden.
Wahrscheinlich nicht das prächtigste Stück Code, das ihr je gesehen habt, aber ich als Anfänger freu mich drüber =D

Etwas wie

Code: Alles auswählen

c = TinyClock(True)
c.start()

und schon hat man die Mini-Uhr.

Code: Alles auswählen

import os
import time
import datetime

class TinyClock:
    def __init__(self, usingSystemTime=False, h=0, m=0, s=0):
        if usingSystemTime:
            timeList = self.__getSystemTime()
            self.hour = int(timeList[0])
            self.min = int(timeList[1])
            self.sec = int(timeList[2])
            self.usingSystemTime = True
        
        else:
            if h < 0 or h > 23:
                raise ValueError("Inappropriate value for hour")
            if m < 0 or m > 59:
                raise ValueError("Inappropriate value for minute")
            if s < 0 or s > 59:
                raise ValueError("Inappropriate value for second")
            
            self.hour = h
            self.min = m
            self.sec = s
            self.usingSystemTime = False
    
    
    def __getSystemTime(self):
        # sample date:
        # 2010-12-13 23:21:20.169001
        dateString = str(datetime.datetime.now()) # datetime obj. --> string
        dateList = dateString.split(" ") # splits by space
        timeList = dateList[1].split(".") # split until all that is left
        timeList = timeList[0].split(":") # is hour, minute, and second
        return timeList
    
    
    def __timeToString(self):
        hStr = str(self.hour)
        mStr = str(self.min)
        sStr = str(self.sec)
        
        if self.hour < 10:
            hStr = "0"+ hStr
        if self.min < 10:
            mStr = "0"+ mStr
        if self.sec < 10:
            sStr = "0"+ sStr
        
        return hStr+":"+mStr+":"+sStr
    
    
    def __updateTime(self):
        if self.usingSystemTime:
            timeList = self.__getSystemTime()
            self.hour = int(timeList[0])
            self.min = int(timeList[1])
            self.sec = int(timeList[2])
        else:
            self.sec += 1
            self.__checkForChanges()
    
    
    def __checkForChanges(self):
        if self.sec > 59:
            self.min += 1
            self.sec = 0
            if self.min > 59:
                self.hour += 1
                self.min = 0
                if self.hour > 23:
                    self.hour = 0
    
    
    def __clearConsole(self):
        if os.name == "posix":
            os.system("clear")
        elif os.name in ("dos", "nt", "ce"):
            os.system("cls")
        else:
            print(80*"\r\n")
    
    
    def start(self):
        while True:
            self.__clearConsole()
            currTime = self.__timeToString()
            print(currTime)
            self.__updateTime()
            time.sleep(1.0)

Wenn ihr Verbesserungsvorschläge zur Struktur oder sonst etwas habt, nur her damit.

[BlackJack]

@sisu: Man sollte Klassen -- zumindest in Python 2.x -- immer explizit von `object` erben lassen, damit man eine "new style"-Klasse bekommt, bei der zum Beispiel so etwas wie `property()` funktioniert.

Die Namen der Methoden entsprechen nicht der kleinbuchstaben_mit_unterstrichen Konvention.

Die `__init__()` ist mir zu "überladen". Wenn man eine Funktion hat, die anhand eines Wertes entscheidet etwas völlig anderes mit dem Rest der Argumente zu machen oder sie zu ignorieren, dann hat man in der Regel zwei Funktionen in eine gesteckt. Ich würde hier die `__init__()` für die flexiblere Variante benutzen, also für die freie übergabe von Stunden, Minuten, und Sekunden und eine Klassenmethode schreiben, die ein `TinyClock`-Exemplar mit der aktuellen Zeit erstellt.

Die doppelten führenden Unterstriche solltest Du durch einen Unterstrich ersetzen. Die Klasse wird ja nicht als Mixin-Klasse verwendet, also gibt es auch keinen Grund Namenskollisionen zu befürchten.

In dem ``if``- und dem ``else``-Zweig in der `__init__()` steht viel zu viel. Es hätte ein ``if`` gereicht das bei `usingSystemTime` die Namen `h`, `m`, `s` an die aktuelle Zeit bindet. Überprüfen der Werte und zuweisen an die Attribute kann man doch in beiden Fällen machen, benötigt also auch keine Fallunterscheidung.

Überprüfen eines Wertes auf einen Bereich kann man "schöner" schreiben, da man in Python Vergleichsoperatoren auch "verketten kann". ``not (0 <= h <= 23)`` ist das gleiche wie ``h < 0 or h > 23``.

Warum kürzt Du `hour`, `min`, und `sec` bei den Argumenten mit einem Buchstaben ab? Und ist `hours`, `minutes`, und `seconds` wirklich zu viel Schreibarbeit?

Ermitteln der aktuellen Zeit machst Du sehr umständlich und eventuell auch nicht besonders robust. Das Objekt erst in eine Zeichenkette umzuwandeln, um dann daraus umständlich wieder die Einzelteile rauszukratzen muss nicht sein. Schau Dir doch mal an was so ein `datetime`-Objekt für Attribute hat.

Bei `__timeToString()` solltest Du Dir mal Zeichenkettenformatierung mittels ``%``-Operator oder `format()`-Methode anschauen. Das ist im Grunde ein Einzeiler. Und die Methode würde sich dann als Implementierung für `__str__()` eignen.

In `__updateTime()` und `__checkForChanges()` machst Du selbst Sachen, für die es schon fertige Lösungen gibt. Man kann mit den Objekten im `datetime`-Modul auch rechnen, also zum Beispiel addieren und abziehen. Du solltest aber nicht `time.sleep()` benutzen um selber eine Zeit hoch zu zählen, denn die Funktion ist nicht genau und so kann Deine Uhr mit jeder Sekunde ein klein wenig mehr falsch gehen. Besser ist es in jedem Fall immer die aktuelle Zeit zu ermitteln, die Startzeit der Uhr abzuziehen und dann die vom Benutzer angegeben Stunden, Minuten, und Sekunden zu addieren. Dann braucht man im laufenden Betrieb der Uhr keine Fallunterscheidung mehr ob man nun eine Stoppuhr oder eine "normale" Uhr hat -- der Code ist immer der selbe.

`__clearConsole()` und `start()` sind Benutzerinteraktion also GUI bzw. in diesem Fall TUI, dass gehört nicht in die Klasse. Der Code dafür sollte von der Programmlogik getrennt sein, damit man zum Beispiel mehrere Uhren gleichzeitig laufen lassen kann, oder sie in einer GUI anzeigen kann.

Wenn man periodisch die Zeit ermittelt und darstellt, sollte man bei einer Uhrauflösung von einer Sekunde nicht auch eine Sekunde für das Update-Intervall wählen, weil es dann in "Grenzfällen" passieren kann, dass die Uhr in der Anzeige mal eine Sekunde überspringt. In solchen Fällen hat es sich bewährt nur die Hälfte der Uhrauflösung zu warten, bevor man die Anzeige aktualisiert.

[sisu]

Man sollte Klassen -- zumindest in Python 2.x -- immer explizit von `object` erben lassen, damit man eine "new style"-Klasse bekommt, bei der zum Beispiel so etwas wie `property()` funktioniert.

Ah, hab ich vergessen zu schreiben, der Code ist für Python ab Version 3 - aber das konnte man wahrscheinlich der Schreibweise von print() entnehmen ^^ Ich verstehe nicht so recht, was du mir sagen willst. Hab mal in der Doku nachgeschlagen, da wird in Kapitel 2 unter Built-in functions eine Funktion property() aufgeführt, aber warum sollte die denn nicht da sein, wenn man nicht explizit von object erben lässt? Von was wird dann geerbt?

Die Namen der Methoden entsprechen nicht der kleinbuchstaben_mit_unterstrichen Konvention.

Ich bin Fan von CamelCase
... hm, ist das sehr schlimm, wenn ich die Unterstriche nicht benutze? In dem Style Guide PEP steht CamelCase doch als zugelassen drin.

Die `__init__()` ist mir zu "überladen". Wenn man eine Funktion hat, die anhand eines Wertes entscheidet etwas völlig anderes mit dem Rest der Argumente zu machen oder sie zu ignorieren, dann hat man in der Regel zwei Funktionen in eine gesteckt.

Die finde ich auch furchtbar, aber so wie ich das bisher gelesen habe, ist es in Python nicht möglich, mehrere Konstruktoren zu definieren, und ich wusste mir nicht anders zu helfen als durch if-Abfragen.

Die doppelten führenden Unterstriche solltest Du durch einen Unterstrich ersetzen. Die Klasse wird ja nicht als Mixin-Klasse verwendet, also gibt es auch keinen Grund Namenskollisionen zu befürchten.

Ah, hab mich vertan, wollte diese Methoden eigentlich als privat markieren. Dann war's also doch nur ein Unterstrich.

In dem ``if``- und dem ``else``-Zweig in der `__init__()` steht viel zu viel. Es hätte ein ``if`` gereicht das bei `usingSystemTime` die Namen `h`, `m`, `s` an die aktuelle Zeit bindet. Überprüfen der Werte und zuweisen an die Attribute kann man doch in beiden Fällen machen, benötigt also auch keine Fallunterscheidung.

Jetzt, wo du es sagst. Ist mir gestern Abend um 12 nicht mehr aufgefallen, als ich das hässliche If hingefriemelt hab.

Überprüfen eines Wertes auf einen Bereich kann man "schöner" schreiben, da man in Python Vergleichsoperatoren auch "verketten kann". ``not (0 <= h <= 23)`` ist das gleiche wie ``h < 0 or h > 23``.

Das wusste ich nicht. Finde ich elegant und werde ich mir merken!

Warum kürzt Du `hour`, `min`, und `sec` bei den Argumenten mit einem Buchstaben ab? Und ist `hours`, `minutes`, und `seconds` wirklich zu viel Schreibarbeit?

Mit den Einzelbuchstaben wollte ich deutlich machen, dass es sich nur um lokale Variablen handelt. min und sec sollte ich wohl wirklich anders nennen, schon wegen der Kollision mit der Funktion min()...

Ermitteln der aktuellen Zeit machst Du sehr umständlich und eventuell auch nicht besonders robust. Das Objekt erst in eine Zeichenkette umzuwandeln, um dann daraus umständlich wieder die Einzelteile rauszukratzen muss nicht sein. Schau Dir doch mal an was so ein `datetime`-Objekt für Attribute hat.

Kam mir ehrlich gesagt einfach nicht in den Sinn, das nachzuschauen - das datetime-Objekt war für mich wie ne Blackbox, die ich möglichst schnell wieder verlassen wollte, deshalb sogleich das Umwandeln in String und das Weitermachen mit einigermaßen bekannten Inhalten ^^:

In `__updateTime()` und `__checkForChanges()` machst Du selbst Sachen, für die es schon fertige Lösungen gibt. Man kann mit den Objekten im `datetime`-Modul auch rechnen, also zum Beispiel addieren und abziehen. Du solltest aber nicht `time.sleep()` benutzen um selber eine Zeit hoch zu zählen, denn die Funktion ist nicht genau und so kann Deine Uhr mit jeder Sekunde ein klein wenig mehr falsch gehen. Besser ist es in jedem Fall immer die aktuelle Zeit zu ermitteln, die Startzeit der Uhr abzuziehen und dann die vom Benutzer angegeben Stunden, Minuten, und Sekunden zu addieren. Dann braucht man im laufenden Betrieb der Uhr keine Fallunterscheidung mehr ob man nun eine Stoppuhr oder eine "normale" Uhr hat -- der Code ist immer der selbe.

Okay, sowas muss man wohl einfach mal gesagt bekommen, damit man aufhört, Räder neu zu erfinden...

Wenn man periodisch die Zeit ermittelt und darstellt, sollte man bei einer Uhrauflösung von einer Sekunde nicht auch eine Sekunde für das Update-Intervall wählen, weil es dann in "Grenzfällen" passieren kann, dass die Uhr in der Anzeige mal eine Sekunde überspringt. In solchen Fällen hat es sich bewährt nur die Hälfte der Uhrauflösung zu warten, bevor man die Anzeige aktualisiert.

Ja, dieses Springen ist mir beim Testen auch aufgefallen, aber auf die Idee, das Aktualisieren hochzuschrauben, bin ich nicht gekommen *hust*


Danke dir für deine Mühe, eine solche Code-Analyse zu schreiben

[BlackJack]

@sisu: Die Funktion `property()` gibt es natürlich immer, aber die funktioniert nur im Zusammenhang mit "new style"-Klassen. In Python 3.x erben alle Klassen implizit von `object`, in 2.x nur wenn man es explizit hinschreibt.

CamelCase ist nicht sehr schlimm, aber es liest sich halt komisch.

Man kann nur eine `__init__()` schreiben, aber natürlich beliebig viele andere Methoden und dabei halt auch Klassenmethoden oder statische Methoden, die ein Exemplar zurückgeben. Ein Beispiel dafür benutzt Du ja schon selbst: `datetime.now()`.

Ob ein Name in einer Method lokal ist, sieht man doch in der Regel daran, ob ein ``self.`` davor steht oder nicht. Ausserdem sollten weder Argumentlisten noch Funktionen so umfangreich werden, dass man da durcheinander kommen könnte.

Mal ein Versuch von mir:

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import sys
from collections import namedtuple
from datetime import datetime as DateTime, timedelta as TimeDelta
from time import sleep

HMS = namedtuple('HMS', 'hours minutes seconds')


def time_delta2hms(time_delta):
    minutes, seconds = divmod(time_delta.seconds, 60)
    hours, minutes = divmod(minutes, 60)
    return HMS(hours % 60, minutes, seconds)


class Clock(object):
    def __init__(self, hours=0, minutes=0, seconds=0):
        self.start = DateTime.now()
        self.offset = TimeDelta(hours=hours, minutes=minutes, seconds=seconds)
    
    def __str__(self):
        return '%02d:%02d:%02d' % self.get_time()
    
    def get_time(self):
        return time_delta2hms(DateTime.now() - self.start + self.offset)
    
    @classmethod
    def from_system_time(cls):
        now = DateTime.now()
        return cls(now.hour, now.minute, now.second)


def main():
    clock = Clock.from_system_time()
    while True:
        sys.stdout.write('\r' + str(clock))
        sys.stdout.flush()
        sleep(0.5)


if __name__ == "__main__":
    main()