Code-Kritik: dieser Code soll schöner werden

[Froc]

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]

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]

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

[numerix]

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]

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]

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]

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]

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]

Keys müssen tuple oder strings sein

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