Code-Kritik: dieser Code soll schöner werden

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.
Froc
User
Beiträge: 2
Registriert: Montag 11. August 2008, 19:38

Code-Kritik: dieser Code soll schöner werden

Beitragvon Froc » Montag 11. August 2008, 20:16

Hallo zusammen,
ich hab vor kurzen mit Python als zweite Sprache nach Java angefangen und mein erstes kleines Programm läuft nach einigem Hin und Her auch endlich. Aber ich bin noch nicht so ganz zufrieden und glaub, das geht auch schöner. Da es leider recht schwierig ist, den eigenen funktionierenden Code zu verbessern, insbesondere als Frischling, hab ich mir gedacht, zeig ich mein Erstlingswerk mal einem versierten Publikum.

Zum Problem: es geht um eine kurze Würfelsimulation. Man hat X(=dices) Würfel, und bei einer 6 darf man noch einmal Würfeln, und alle Augen zusammenzählen [dicer]
Zur statistischen Auswertung wird die so generierte Liste nach gezeigter Augenanzahl neusortiert [lister], und in Prozente umgerechnet [percenteger].
Um das ganze dann statistisch aufzubereiten, wird das mehrmalige Aufrufen mit rounder(iterations,dices) gleich mitgeliefert.
rounder(20,8) liefert also eine liste die bei 1 beginnend die Wahrscheinlichkeit der Augenanzahl bei 20 durchläufen anzeigt.

negative Eingaben und ähnlichen Krimskrams hab ich noch nich reingetan - quasi Coder-only Programm

Also stürzt euch drauf, und zeigt, dass ihr es schöner könnt!
insbesondere die for-Schleifen über Listen und das addieren von Listenelementen geht wohl schöner.

Gruß und Danke im Vorraus - Froc

Code: Alles auswählen

import random
def dicer(dices):
    #throws 6sided dices,
    #and when a six shows up, rolls again with that dice,
    #adding the outcome
   dicelist=[]
   for i in range(dices):
      n=random.randint(1,6)
      dicelist.append(n)
   for i in range(len(dicelist)):
      lower=6
      while dicelist[i]==lower:
         dicelist[i]=dicelist[i]+random.randint(1,6)
         lower=lower+6
   return dicelist

def lister(dicelist):
    #gets a outcome list from dicer
    #and gives a list, with the the shown added eyes
    #[4,5,6] will mean: 4 rolls with 1 eye - 5 rolls with 2 eyes, 6 rolls with 3 eyes
   eyelist=[]
   for i in range(max(dicelist)):
      eyelist.append(0)
   for j in range(len(dicelist)):
      eyelist[dicelist[j]-1]=1+eyelist[dicelist[j]-1]
   return eyelist

def percenteger(liste):
    #normalices the appereances of rolls
    #[1,2,3] will give: [1/6, 1/3, 1/2]in floating
   outlist=[]
   for i in liste:
      outlist.append(float(i)/float(sum(liste)))
   return outlist     
            
def rounder(count, dices):
    #multiple iterations of the three programms above
    #to show statistic appereances with same amount of dices
   outlist=[]
   boxlist=[]
   maximum=0
   for i in range(count):
      a=percenteger(lister(dicer(dices)))
      while maximum<len(a):
         maximum=maximum+1
         outlist.append(0.0)
      boxlist.append(a)
   
   for i in range(len(boxlist)):
      for j in range(len(boxlist[i])):
         outlist[j]=boxlist[i][j]+outlist[j]
   
   for i in range(len(outlist)):
         outlist[i]=outlist[i]/count
   return outlist

PS: ihr braucht natürlich nicht den ganzen Code verschönern, ich freu mich auch über Zeilen und generelle Anmerkungen
BlackJack

Beitragvon BlackJack » Montag 11. August 2008, 20:58

Möglichst nahe am Original würde ich es (ungetestet) so machen:

Code: Alles auswählen

from __future__ import division
from itertools import imap
from random import randint


def roll(dice_count):
    result = list()
    for dummy in xrange(dice_count):
        total = 0
        roll = 6
        while roll == 6:
            roll = randint(1, 6)
            total += roll
        result.append(total)
    return result


def histogram(roll_results):
    result = [0] * max(roll_results)
    for eyes in roll_results:
        result[eyes - 1] += 1
    return result


def percentages(values):
    total = sum(values)
    return [v / total for v in values]


def foo(repeat_count, dice_count):
    boxes = [percentages(histogram(roll(dice_count)))
             for dummy in xrange(repeat_count)]
    result = [0.0] * max(imap(len, boxes))
    for box in boxes:
        for i, roll in enumerate(box):
            result[i] += roll
    return [x / repeat_count for x in result]


Die Namen im Original gefallen mir nicht, das sind eher welche für Klassen.

Du arbeitest zu viel mit Indexen, zum Beispiel in der zweiten Schleife in `lister()`. Das ist bei mir die `histogram()`.

Insgesamt solltest Du überlegen die Listen bei den Funktionen (ausser vielleicht bei `dicer()`/`roll()`) durch Dictionaries zu ersetzen, welche die Augenzahl auf die jeweiligen Werte abbildet.
EyDu
User
Beiträge: 4866
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Beitragvon EyDu » Montag 11. August 2008, 21:12

Ich habe meinen Code nicht getestet, musst du zur Not selber etwas suchen:

Code: Alles auswählen

import random

def dicer(dices):
    dicelist = [random.randint(1,6) for i in range(dices)]

    for i,x in enumerate(dicelist):
        lower = 6
        while dicelist[i]==lower:
            dicelist[i] += random.randint(1,6)
            lower += 6
   
    return dicelist

def lister(dicelist):
    eyelist=[0]*max(dicelist)
   
    for i,x in enumerate(dicelist):
        eyelist[x-1] += 1
   
    return eyelist

def percenteger(liste):
    return [float(i)/sum(liste) for i in liste]
               
def rounder(count, dices):
    outlist = []
    boxlist = []
    maximum = 0
   
    for i in range(count):
        a = percenteger(lister(dicer(dices)))
       
        while maximum<len(a):
            maximum += 1
            outlist.append(0.0)
       
        boxlist.append(a)
   
   
    for box in boxlist:
        for i,x in enumerate(boxlist[i]):
            outlist[i] += x
   
    return [x/count for x in outlist]


Die erste (geht in einer Loop) und letzte Funktion kann man noch etwas zusammenfassen, mir ist aber die Lust ausgegangen ;-)
Benutzeravatar
numerix
User
Beiträge: 2696
Registriert: Montag 11. Juni 2007, 15:09

Beitragvon numerix » Montag 11. August 2008, 21:21

Ich picke mal ein paar Sachen raus (alles wäre zu viel ...):

dicer():
Zeile 6-10 lässt sich mit einer list comprehension "verschönern":

Code: Alles auswählen

dicelist=[random.randint(1,6) for n in range(dices)]


Der Teil darunter mit den Indizes ist nicht elegant. Überhaupt - hat BlackJack ja schon geschrieben, zu viele Indizes.

Eine Alternative für den dicer() wäre, nicht erst mit allen Würfeln einmal zu würfeln und danach in einem zweiten Durchgang mit den Sechsern entsprechend weiter zu machen, sondern mit jedem Würfel gleich einzeln entsprechend zu verfahren. Das sähe dann so aus:

Code: Alles auswählen

def dicer(dices):
    dicelist = []
    for n in range(dices):
        eyes = random.randint(1,6)
        while not eyes % 6:
            eyes += random.randint(1,6)
        dicelist.append(eyes)
    return dicelist


Zeile 21-23 ist "hintenrum durchs Auge":
Was du willst, ist

Code: Alles auswählen

eyelist=[0]*max(dicelist)


Allerdings sollte man das gar nicht so angehen. Ein dictionary wäre hier die geeignete Datenstruktur.

Edit: Da war ich wohl was zu langsam ...
audax
User
Beiträge: 830
Registriert: Mittwoch 19. Dezember 2007, 10:38

Beitragvon audax » Montag 11. August 2008, 22:12

Code: Alles auswählen

from __future__ import division
from itertools import groupby
from functools import partial
from random import randint

def roll_dices(dices):
    """ throws 6sided dices,
        and when a six shows up, rolls again with that dice,
        adding the outcome
    """
    def roll():
        _roll = partial(randint, 1, 6)
        d = _roll()
        if d == 6:
            d += _roll()
        return d
    return [roll() for _ in xrange(dices)]

def count_eyes(dicelist):
    """ gets a outcome list from roll_dices
        and gives a list, with the the shown added eyes
        [4,5,6] will mean: 4 rolls with 1 eye - 5 rolls with 2 eyes, 6 rolls with 3 eyes
    """
    return dict(
                (k-1, len(list(v)))
                    for k, v in groupby(sorted(dicelist)))

def histogramm(rolls):
    """ normalices the appereances of rolls
        [1,2,3] will give: [1/6, 1/3, 1/2]in floating
    """
    eyes = sum(rolls)
    return [d/eyes for d in rolls]
               
def rounder(count, dices):
    """ multiple iterations of the three programms above
        to show statistic appereances with same amount of dices
    """
    outlist=[]
    boxlist=[]
    maximum=0
    for i in xrange(count):
        a = histogramm(count_eyes(roll_dices(dices)))
        while maximum < len(a):
            maximum = maximum+1
            outlist.append(0.0)
        boxlist.append(a)
   
    for i in xrange(len(boxlist)):
        for j in xrange(len(boxlist[i])):
            outlist[j]=boxlist[i][j]+outlist[j]
   
    for i in xrange(len(outlist)):
            outlist[i]=outlist[i]/count
    return outlist


Bis auf die letzte Funktion mal angeschaut :]
BlackJack

Beitragvon BlackJack » Montag 11. August 2008, 22:46

Kann sein, dass es jetzt auch einfach zu spät ist, aber eigentlich sollte es doch keinen Unterschied machen, ob man das Spielchen x mal mit y Würfen wiederholt und davon dann die Mittelwerte berechnet, oder ob man gleich x*y Würfe macht!? Dann würde das, wieder ungetestet und diesmal mit Dictionary, auch kürzer mit diesen beiden Funktionen gehen:

Code: Alles auswählen

from __future__ import division
from collections import defaultdict
from random import randint


def roll(sides=6):
    result = 0
    last_roll = sides
    while last_roll == sides:
        last_roll = randint(1, sides)
        result += last_roll
    return result


def foo(repeat_count):
    histogram = defaultdict(int)
    total = 0
    for eyes in (roll() for dummy in xrange(repeat_count)):
        histogram[eyes] += 1
        total += eyes
    return dict((eyes, eyes * count / total)
                for eyes, count in histogram.iteritems())
Froc
User
Beiträge: 2
Registriert: Montag 11. August 2008, 19:38

Beitragvon Froc » Dienstag 12. August 2008, 11:02

da bin ich ja ehrlich überrascht wie schnell das ging!
Das mit den Indizes war wohl eine kleine Trotzreaktion, da es zwischendurch Probleme gab, Listenelemente zu addieren und nicht Listen zu konkatenieren. Aber mit

Code: Alles auswählen

for eyes in roll_results:
        result[eyes - 1] += 1

is das ja nichtmehr das Problem.

Code: Alles auswählen

def roll(sides=6):
gefällt mir auch, wenn man denn mal keine 6-seitigen Würfel nimmt.

Für den Hinweis auf xrange() bin ich auch Dankbar, wenngleich ich mich schon ein bisschen gefreut hatte, als mein PC gestern doch mal richtig für mich gerattert hat *g*

Auch die Idee, jeden Würfel erst "zu Ende" zu würfeln, und nicht die Liste ein zweites mal nach 6ern durchzugehen hätte mir selbst kommen müssen. Da bin ich wohl zu nah am eigentlichen Würfeln beglieben.
Von

Code: Alles auswählen

from __future__ import division
bin ich auch begeistert, das Geschachtel von float() war wenig ansehnlich.

Ob es keinen Unterschied mach, wenn man
das Spielchen x mal mit y Würfen wiederholt und davon dann die Mittelwerte berechnet, oder ob man gleich x*y Würfe macht
müsst ich erst Prüfen, aber ich denk schon, weil ja mit weniger Würfeln die Kette der sechsen schneller abreist - zumindest bei kleinen Würfelzahlen. Aber genau darum geht es: mehr Würfel oder öfters wiederholen.

Ein Dictionary solls mMn nich werden, bin mit der 2 dimensionalen Liste eigentlich ganz zufrieden. Muss aber sagen, dass ich hinter die Dictionarys noch nich so ganz gekommen bin - kommt aber sicher noch.

Code: Alles auswählen

for box in boxlist:
        for i,x in enumerate(boxlist[i]):
            outlist[i] += x

hab ich nich ganz verstanden, gilt dann einfach, dass i=x, nur i ist die Zahl der Iteration und x das Objekt?

Ach und den OMG-klar! Award hat natürlich die Zeile

Code: Alles auswählen

eyelist=[0]*max(dicelist)
gewonnen.

In dem Sinne Danke und bis bald, Froc
DasIch
User
Beiträge: 2402
Registriert: Montag 19. Mai 2008, 04:21
Wohnort: Berlin

Beitragvon DasIch » Dienstag 12. August 2008, 14:42

Froc hat geschrieben:Muss aber sagen, dass ich hinter die Dictionarys noch nich so ganz gekommen bin - kommt aber sicher noch.

Was ist den genau das Problem? Im Prinzip ist es einfach, ein Dictionary ist unsortiert. Keys müssen tuple oder strings sein und wenn man darüber iteriert bekommt man die keys. Desweiteren gibt es einige Methoden die man mit dir(dict) herausbekommt.
Zap
User
Beiträge: 533
Registriert: Freitag 13. Oktober 2006, 10:56

Beitragvon Zap » Dienstag 12. August 2008, 14:48

Froc hat geschrieben:Keys müssen tuple oder strings sein

Jedes Objekt das hashable ist, kann als key verwendet werden..

Wer ist online?

Mitglieder in diesem Forum: Google [Bot]