Das deutsche Python-Forum

Hallo,
das passiert wenn einen die Langeweile packt...
Dachte mir ich implementiere mal eine kleine Dictionary-Klasse und das
ist dabei rausgekommen...
Hoffe

Code: Alles auswählen

"""My own dictionary class"""

class Dictionary ():
    "This class is an alternative dictionary class."
    def __init__ (self):
        "The __init__ function creates the lists of the dictionary."
        self.list_of_keys=[]
        self.list_of_values=[]
    def AddToDictionary (self,keys,values,position=None):
        """This function adds values and keys to your dictionary.
           Keys and Values have to be lists.
           The length of the lists MUST be the same."""
        if len(keys) == len(values):
            pass
        if len(keys) > len(values):
            raise ValueError, "Too much keys."
        if len(keys) < len(values):
            raise ValueError, "Too much values."
        if position is None :
            for i in range(len(keys)):
                if self.HasKey(keys[i]) == 0:
                    pass
                if self.HasKey(keys[i]) == 1:
                    raise ValueError, "Key already exists!"
                self.list_of_keys.insert(len(self.list_of_keys)+len(keys),keys[i])
            for a in range(len(values)):
                if self.HasValue(values[a]) == 0:
                    pass
                if self.HasValue(values[a]) == 1:
                    raise ValueError, "Value already exists!"
                self.list_of_values.insert(len(self.list_of_values)+len(values),values[a])
        if position is not None :
            for i in range(len(keys)):
                if self.HasKey(keys[i]) == 0:
                    pass
                if self.HasKey(keys[i]) == 1:
                    raise ValueError, "Key already exists!"
                self.list_of_keys.insert(position+len(keys),keys[i])
            for a in range(len(values)):
                if self.HasValue(values[a]) == 0:
                    pass
                if self.HasValue(values[a]) == 1:
                    raise ValueError, "Value already exists!"
                self.list_of_values.insert(position+len(values),values[a])
    def HasKey (self,key):
        "Finds out, if the Dictionary has the Key you are searching for."
        try:
            self.list_of_keys.index(key)
            return 1
        except ValueError:
            return 0
    def HasValue (self,value):
        "Finds out, if the Dictionary has the Value you are searching for."
        try:
            self.list_of_values.index(value)
            return 1
        except ValueError:
            return 0
    def GiveMeValue (self,key):
        "Gives you the value of the key you has entered."
        if self.HasKey(key) == 1:
            return self.list_of_values[self.list_of_keys.index(key)]
        if self.HasKey(key) == 0:
            raise ValueError, "This key doesn't exist."
    def ShowDictionary(self):
        for i in range(len(self.list_of_keys)):
            print i,"key",self.list_of_keys[i],"value",self.list_of_values[i]

Mir ist übrigens bewusst, dass das Dictionary in Python bereits wunderbar
funktioniert...

MfG Jonas

Boah, ist das krank und eklig. Ich glaub ich werd blind. Da weiss man ja gar nicht wo man mit der Kritik anfangen soll. Am besten komplett in die Tonne.

Du solltest dir einmal PEP-8 zu Gemüte führen

Code: Alles auswählen

def HasKey (self,key):
        "Finds out, if the Dictionary has the Key you are searching for."
        try:
            self.list_of_keys.index(key)
            return 1
        except ValueError:
            return 0

Wohl eher:

Code: Alles auswählen

def has_key(self, key):
    return key in self.list_of_keys

Für ShowDictionary wäre es sicher geschickter, __str__ bzw. __repr__ zu nutzen.

Redprince hat geschrieben:Für ShowDictionary wäre es sicher geschickter, __str__ bzw. __repr__ zu nutzen.

Und im übrigen auch alle anderen __*__ Hooks benutzen. Ich sehe zwar nich unbedingt Sinn daran, aber wenn du unbedingt willst, dann kannst du natürlich deine eigene Hashtable schreiben, jedoch sollte sie zumindest die Komfortabilität des eingebauten dicts erreichen.

Im Übrigen schliesse ich mich BJ an

Ich glaub, dass ich mich langsam beruhigt habe.. Also:

Code: Alles auswählen

if len(keys) == len(values):
    pass
if len(keys) > len(values):
    raise ValueError, "Too much keys."
if len(keys) < len(values):
    raise ValueError, "Too much values."

Abfragen, in deren Rumpf nur pass steht, sind unnütz. Bei ähnlichen Bedingungen bietet sich auch elif an:

Code: Alles auswählen

if len(keys) > len(values):
    raise ValueError
elif len(keys) < len(values):
    raise ValueError

Und weil du hier len() zweimal hintereinander aufrufst und sich die beiden Listen sicher noch nicht verändert haben, bindest du das Ergebnis vor den Abfragen an eine Variable und testet dann mit der.

Python kennt True und False, die Verwendung von 1 oder 0 anstelle der Schlüsselworte ist unübersichtlich und unpythonisch.

Code: Alles auswählen

for a in range(len(values)):
    if self.HasValue(values[a]) == 0:
        pass
    if self.HasValue(values[a]) == 1:
        raise ValueError, "Value already exists!"

Siehe enumerate()!

@redprince: An der Stelle ist `enumerate()` nicht nötig, da reicht einfaches Iterieren über `values`. Und `True`/`False` sind erst ab Python 3.0 wirklich Schlüsselwörter. Davor kann man noch Leser mit einem ``True, False = False, True`` verwirren.

@Jonas: Hast Du das auch mal ausprobiert? Zeile 26 und 32 sind wohl die umständlichste Art effektiv ein einfaches `append()` auf einer Liste zu machen, die ich bisher gesehen habe. Und die Bedeutung von `position` ist mir auch irgendwie noch nicht klar. Das ist jedenfalls nicht die Position ab der eingefügt wird, weil die zusätzlich noch von der Länge der einzufügenden Daten abhängt. WTF!? Und warum müssen bei Deinem "Dictionary" auch *Werte* eindeutig sein? Ist Dir das klar?

Moin,

sieht ja heiß aus. Dann wollen wir mal:

Code: Alles auswählen

class Dictionary ():

Leite dein Dictionary wenigstens von object ab: Stichwort New-Style-Classes.

Code: Alles auswählen

    def AddToDictionary (self,keys,values,position=None):

Du kannst deine Methoden natürlichen nennen, wie du willst. Aber Konvention wäre, wenigstens den Methodennamen klein zu beginnen. Besser wäre noch add_to_dictionary...

Code: Alles auswählen

        if position is None :
            for i in range(len(keys)):
                if self.HasKey(keys[i]) == 0:
                    pass
                if self.HasKey(keys[i]) == 1:
                    raise ValueError, "Key already exists!"
                self.list_of_keys.insert(len(self.list_of_keys)+len(keys),keys[i])
            for a in range(len(values)):
                if self.HasValue(values[a]) == 0:
                    pass
                if self.HasValue(values[a]) == 1:
                    raise ValueError, "Value already exists!"
                self.list_of_values.insert(len(self.list_of_values)+len(values),values[a])

if-Bedingung mit pass kannst du komplett streichen, wie schon angemerkt wurde.

Du kannst direkt über die keys iterieren. Den Index brauchst du ja nicht; also: for key in keys. Dann sparst du dir den x-fachen Indexzugriff.

Die letzte Zeile funktioniert auch nur by accident: list_of_values enthalte drei Werte. Es werden zwei weitere Werte hinzugefügt. Du willst nun den ersten der zwei neuen Werte ans Ende stellen. Der Index hierfür wäre drei. Du errechnest aber den Index fünf, der eigentlich zu weit ist. Die insert-Methode fügt in diesem Fall aber automatisch ans Ende und ignoriert den eigentlich falschen Index.

Aber wozu append benutzen, wenn's auch kompliziert geht

.

Code: Alles auswählen

        if position is not None :
            for i in range(len(keys)):
                if self.HasKey(keys[i]) == 0:
                    pass
                if self.HasKey(keys[i]) == 1:
                    raise ValueError, "Key already exists!"
                self.list_of_keys.insert(position+len(keys),keys[i])
            for a in range(len(values)):
                if self.HasValue(values[a]) == 0:
                    pass
                if self.HasValue(values[a]) == 1:
                    raise ValueError, "Value already exists!"
                self.list_of_values.insert(position+len(values),values[a])

Der Code ist redundant zum obigen. Außerden solltest du das als else-Block definieren.

Aber noch besser: Mach doch erst einmal (!) deine Prüfungen und wenn alles ok ist, entscheide dann, ob du append oder insert verwenden willst (je nachdem, ob position None ist oder nicht). So könntest du die Redundanz umgehen.

Soweit mal von mir.

Gruß,
Manuel

Nicht weiter getestet als das was man unten in der `main()` sieht, und die Geschichte mit `position` habe ich mir gespart:

Code: Alles auswählen

class Dictionary(object):
    def __init__(self):
        self._items = list()
    
    def __str__(self):
        result = ['<Dictionary len: %d' % len(self)]
        result.extend(' %d. key: %r value: %r' % (i, k, v)
                      for i, (k, v) in enumerate(self.iteritems()))
        result[-1] += ' >'
        return '\n'.join(result)
    
    def __iter__(self):
        return (key for key, value in self.iteritems())
    
    def __len__(self):
        return len(self._items)
    
    def __contains__(self, key):
        return key in iter(self)
    
    def __getitem__(self, key):
        for key2, value in self.iteritems():
            if key == key2:
                return value
        raise KeyError('%r not found' % key)
    
    def __setitem__(self, key, value):
        if key in self:
            raise ValueError("key already exists")
        elif value in self.itervalues():
            raise ValueError("value already exists")
        else:
            self._items.append((key, value))
    
    def has_value(self, value):
        return value in self.itervalues()
    
    def itervalues(self):
        return (value for key, value in self.iteritems())
    
    def iteritems(self):
        return iter(self._items)
    
    def add_items(self, items):
        for key, value in items:
            self[key] = value
    
    iterkeys = __iter__
    has_key = __contains__


def main():
    d = Dictionary()
    print d
    d['answer'] = 42
    print d
    d.add_items([(1, 2), ('a', 'b')])
    print d
    try:
        d['answer'] = 23
    except ValueError:
        print 'Ooops'
    try:
        d['spam'] = 42
    except ValueError:
        print 'Ooops'

nabend.

@BlackJack:
Mag ja alles sein, dass mein Code schlecht ist oder nicht der PEP-8 Regelung entspricht, aber ich finde das ist kein Grund pampig zu werden, denn das:

Boah, ist das krank und eklig. Ich glaub ich werd blind. Da weiss man ja gar nicht wo man mit der Kritik anfangen soll. Am besten komplett in die Tonne.

schlägt ja wohl dem Fass den Boden aus! Immer schön konstruktiv bleiben.
zur `position` Bedeutung:
Ich wollte nicht, dass neu hinzugefügte Werte und Schlüssel in einer extra Klammer stehen:

Code: Alles auswählen

((1,2),(3,4))

wie hier zum Beispiel.
Als ich versuchte folgendermaßen zu iterieren

Code: Alles auswählen

for i in range(len(keys)):

habe ich mich gewundert,
denn er hat die Liste, die ich ihm eingab einfach rückwärts iteriert, daher kam ich auf

Code: Alles auswählen

self.list_of_keys.insert(len(self.list_of_keys)+len(keys),keys[i])

um bei der Rückwärtsiteration quasi Platz zu machen. Tut mir leid wenn ich es nicht besser umschreiben kann.
Zu deiner Äußerung, dass meine Werte auch einmalig sein müssen kann ich nur sagen, du hast Recht, das ist Schwachsinn.
Was ist anders an

Code: Alles auswählen

self._items = list()

im Gegensatz zu

Code: Alles auswählen

self._items = []

?
Danke für deine Anregungen im unteren Segment und danke für deine rege Beteiligung viele "Sachen" wie z.B. `iter` oder `join` waren mir noch nicht bekannt, werde ich versuchen einzubauen.

@Redprince:
Danke für deinen Vorschlag oben. Auch die Sache mit dem `pass` stimmt natürlich.

@helduel, cofi:
Danke für die Anregungen.

So abschließend kann ich nur sagen, danke für die vielen Vorschläge zur Verbesserung, werde versuchen sie einzubauen. Wenn mal eine Kritik an euren Beiträgen zu ruppig klingt : Sorry.

MfG (der jetzt schlauere) Jonas

jonas hat geschrieben:Was ist anders an
Code: Alles auswählen
self._items = list() 
im Gegensatz zu
Code: Alles auswählen
self._items = []
?

Im Allgemeinen: nichts. Beides erstellt eine Liste. Der Unterschied ist, dass ``list()`` überschrieben sein könnte (sowieso eine schlechte Idee) und ``[]`` marginal schneller ist. Jedoch ist dafür ``list()`` etwas expliziter und vielleicht auch ein wenig klarer. Unterschied macht es eigentlich keinen besonders großen.

Hallo.
@leonidas:

Danke für deine schnelle Antwort, dieser Punkt ist schonmal abgehakt.

MfG Jonas

Du wolltest doch Kritik...und der von Blackjack muss ich auch zustimmen. Der Code ist einfach völlig grausam. Hier wird einem zwar kompetent geholfen, aber dafür muss man dann eben auch mal den härteren Ton ertragen. So lernt man am Besten

PEP 8, offizielles Tutorial. Sofort!

audax hat geschrieben:Hier wird einem zwar kompetent geholfen, aber dafür muss man dann eben auch mal den härteren Ton ertragen.

BlackJacks Posting war alles mögliche, aber nicht mal im Ansatz kompetente Hilfe. Insofern kann man den "härteren Ton" dieses Postings mit allem möglichen rechtfertigen, aber mit Sicherheit nicht mit der Phrase "kompetente Hilfe".

Man kann vom OP vielleicht verlangen, sich einen "härteren Ton" gefallen zu lassen, wenn er dafür was aus dem betreffenden Posting lernt. Wenn das Posting aber schlichtweg inhaltlos ist, muss man dem OP nicht auch noch einreden, dass der "härtere Ton" zu seinem Besten war.

So lernt man am Besten

Der OP konnte aus BJs Posting höchstens lernen, hier keine Code mehr zwecks Rezension zu posten, weil man eh nur ruppige Antworten erhält.

my 2 cents, im Übrigen teile ich BJs Meinung.

@Jonas: Ja mein erster Beitrag war in der Tat nicht so hilfreich und ziemlich hart Ausgedrückt. Ich wusste aber wirklich nicht wo ich anfangen sollte, weil das eben wirklich nach einem Beispiel aussieht, wo wirklich alles "falsch" ist. Von Stilfragen und klaren Anti-Pattern/Anti-Idiomen, über viel zu umständlichen Code, wo man sich fragt, ob der Programmierer weiss was er da tut, bis zu Fehlern, zumindest wenn ich `position` so interpretiere wie man es intuitiv annehmen würde.

lunar hat geschrieben:
audax hat geschrieben:Hier wird einem zwar kompetent geholfen, aber dafür muss man dann eben auch mal den härteren Ton ertragen.
BlackJacks Posting war alles mögliche, aber nicht mal im Ansatz kompetente Hilfe. Insofern kann man den "härteren Ton" dieses Postings mit allem möglichen rechtfertigen, aber mit Sicherheit nicht mit der Phrase "kompetente Hilfe".

Man kann vom OP vielleicht verlangen, sich einen "härteren Ton" gefallen zu lassen, wenn er dafür was aus dem betreffenden Posting lernt. Wenn das Posting aber schlichtweg inhaltlos ist, muss man dem OP nicht auch noch einreden, dass der "härtere Ton" zu seinem Besten war.

So lernt man am Besten
Der OP konnte aus BJs Posting höchstens lernen, hier keine Code mehr zwecks Rezension zu posten, weil man eh nur ruppige Antworten erhält.

my 2 cents, im Übrigen teile ich BJs Meinung.

Ich sah den Post im Kontext mit BJs, zu dem Zeitpunkt aktuellem, Post in dem er dann hilft.

Für sich gesehen ist der erste Post natürlich nicht seht nützlich

Hey!
Vielen Dank für die vielen Antworten!
@Blackjack: Ich wollte dich nicht verurteilen, aber ich habe eben noch nicht so viel Erfahrung mit python und vorher habe ich eben noch kein Dictionary programmiert. Tut mir leid, will gerne versuchen mich zu verbessern.
@alle andern:
Noch mal danke.

MfG Jonas

Das deutsche Python-Forum

Nur mal eine kleine Dictionary Klasse

Nur mal eine kleine Dictionary Klasse

Re: Nur mal eine kleine Dictionary Klasse