Conway's Game of Life - bitte um haufenweise Kritik

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
da3dalus
User
Beiträge: 9
Registriert: Sonntag 14. Januar 2007, 20:06

Sonntag 14. Januar 2007, 20:25

Hi ihr,
ich bin noch ein blutiger Anfänger in Sachen Python und Programmierung generell. Da mir für sinnvolle Programme die Ideen und vermutliche auch die Fähigkeiten fehlen, habe ich heute aus Spaß an der Freude ein Programm geschrieben, das Conways 'Spiel des Lebens' spielt (wer es nicht kennt: http://de.wikipedia.org/wiki/Conways_Spiel_des_Lebens ). Damit sich das Problem der fehlenden Fähigkeit ändert, würde ich mich freuen, wenn ihr an dem Programm alles kritisiert, was es zu kritisieren gibt (inklusive schlechtem Stil etc.). :D

Code: Alles auswählen

 
#!/usr/bin/env python
# -*- coding: utf8 -*-
import string
import time

def printlife(life, length): #zeigt das momentane Biotop
	for i in range(0, length * length):
		if life[i][0] == 0:
			print ' ',
		elif life[i][0] == 1:
			print 'o',
		if (i + 1) % length == 0:
			print

def getvalue(life, position, length): #zählt die lebenden Zellen in der Nachbarschaft
	if position % length == 0:
		if position == 0:
			return (life[1][0] + life[length][0] +
			life[1 + length][0])
		elif position == length * (length - 1):
			return (life[position - length][0] +
			life[position + 1 - length][0] +
			life[position + 1][0])
		return (life[position - length][0] +
			life[position + 1 - length][0] +
			life[position + length][0] + 
			life[position + 1 + length][0] + 
			life[position + 1][0])
	elif (position + 1) % length == 0:
		if position == length - 1:
			return (life[position - 1][0] +
			life[position + length - 1][0] + 
			life[position + length][0])
		elif position == (length * length) - 1:
			return (life[position - length - 1][0] +
			life[position - length][0] +
			life[position - 1][0])
		return (life[position - length - 1][0] +
			life[position - length][0] + 
			life[position - 1][0] +
			life[position - 1 + length][0] + 
			life[position + length][0])
	if position < length:
		return (life[position - 1][0] + 
			life[position + 1][0] + 
			life[position - 1 + length][0] + 
			life[position + length][0] +
			life[position + length + 1][0])
	elif position > length * (length - 1):
		return (life[position - length - 1][0] +
			life[position - length][0] + 
			life[position - length + 1][0] +
			life[position - 1][0] + 
			life[position + 1][0])
	return (life[position - 1 - length][0] + life[position - length][0] +
		life[position + 1 - length][0] + 
		life[position - 1][0] + 
		life[position + 1][0] +
		life[position + length - 1][0] + 
		life[position + length][0] +
		life[position + length + 1][0])

def evolve(life, length): #entwickelt die nächste Generation
	for position in range(0, length * length - 1):
		if getvalue(life, position, length) < 2:
			life[position].append(0)
		elif getvalue(life, position, length) == 2:
			if life[position][0] == 0:
				life[position].append(0)
			elif life[position][0] == 1:
				life[position].append(1)
		elif getvalue(life, position, length) == 3:
			life[position].append(1)
		elif getvalue(life, position, length) > 3:
			life[position].append(0)
	for position in range(0, length * length- 1):
		del life[position][0]
	return life

def main():
	length = int(raw_input('Welche Seitenlaenge soll das Biotop haben? '))
	life = []
	for i in range(0, length * length):
		life.append([0])
	answer = None
	while answer != '' and answer != 'nein':
		answer = raw_input('Zelle zum Leben erwecken? Wenn ja, welche?'
		'(Koordinaten) ')
		if answer != 'nein' and answer != '':
			life[int(string.split(answer, ',')[0]) - 1 + \
			(int(string.split(answer, ',')[1]) - 1) * length][0] = 1
	nextanswer = int(raw_input('Spiel einfach laufen lassen (1) \
	oder x-te Generation ansehen(x eingeben)? '))
	if nextanswer == 1:
		while True:
			printlife(life, length)
			evolve(life, length)
			time.sleep(1)
			print
	elif nextanswer > 1:
		for generation in range(1, nextanswer):
			evolve(life, length)
		printlife(life, length)
main()
 
Was ich selber noch als unschön empfinde, wofür es mir aber gerade an Konzentration und Ideen mangelt, sind die getvalue-Funktion, die mit ziemlicher Sicherheit schöner und einfacher zu realisieren ist, und die Darstellung der Zellen als Listen. Aber auch ansonsten bin für jede Kritik und jeden Verbesserungsvorschlag dankbar :D
Zuletzt geändert von da3dalus am Dienstag 16. Januar 2007, 17:25, insgesamt 2-mal geändert.
Leonidas
Administrator
Beiträge: 16024
Registriert: Freitag 20. Juni 2003, 16:30
Kontaktdaten:

Sonntag 14. Januar 2007, 21:51

Da ich das inwischen recht oft gemacht habe, fasse ich mich diesmal kurz und scheibe nur mal eine Liste zusammen:
  • Unflexibler Sheband, besser /usr/bin/env python verwenden
  • Kein Encoding angegeben
  • Viel Code auf Modulebene, sowas gehört in eine main()-Funktion
  • Namen nicht PEP8-konform
  • Statt der Inline-Kommentare bei den Funktionen solltest du besser Docstrings verwenden.
  • Hier könnten Klassen dir sicherlich einiges vereinfachen
  • answer sollte nicht 'nix' sein, sondern setz' das lieber auf None.
My god, it's full of CARs! | Leonidasvoice vs Modvoice
Python 47
User
Beiträge: 574
Registriert: Samstag 17. September 2005, 21:04

Sonntag 14. Januar 2007, 22:24

zudem:
  • Codezeilen sind zu lang [wiki]Lange Zeilen im Sourcecode[/wiki]
  • Leerzeichen machen den Code übersichtlicher
  • Ofte Codewiederholungen( if, elif) lassen sich besser zusammenfassen
mfg

Thomas :-)
BlackJack

Sonntag 14. Januar 2007, 22:34

Die Zeilen im Quelltext kleben alle zu dicht aneinander. Mindestens zwischen den Funktionsdefinitionen sollte eine Leerzeile sein, um das etwas übersichtlicher zu gestalten.

`arrays` ist ein ziemlich irreführender Name für eine "Kantenlänge".

Wenn der Hauptcode in einer `main()`-Funktion steckt, dann wirst Du feststellen, dass Du an einigen Stellen auf globale Namen zugreifst, anstatt sie als Argumente an die Funktionen zu übergeben. `life` zum Beispiel.

Die Datenstruktur ist nicht so toll. Besser wäre entweder eine Liste die gleich die Elemente enthält, oder eine Liste mit Listen für die Zeilen. Dann passt die Datenstruktur besser zur Bildschirmausgabe und der Quelltext wird vielleicht auch etwas verständlicher wenn man die Zellen über eine X- und eine Y-Koordinate anspricht.

Von einem Schritt zum nächsten sollte man besser eine neue Liste oder "2D-Liste" erzeugen, statt diese komische Konstruktion die jetzt verwendet wird.

`getvalue()` ist wirklich nicht hübsch. Es wäre besser über die möglichen Koordinaten zu iterieren und die Randfälle einfach durch die Behandlung der entsprechenden Ausnahmen abzufangen.
da3dalus
User
Beiträge: 9
Registriert: Sonntag 14. Januar 2007, 20:06

Montag 15. Januar 2007, 18:01

Ok, erstmal danke für eure Tipps. Zu einigen Sachen habe ich aber noch Fragen:
Leonidas hat geschrieben: [*]Unflexibler Sheband, besser /usr/bin/env python verwenden
Ok, habe ich gemacht. Aber warum ist das besser?
[*]Hier könnten Klassen dir sicherlich einiges vereinfachen.
Stimmt, aber wenn ich beispielsweise eine Klasse für die Felder machen wollte, müsste das Programm ja die Objekte erstellen. Da weiß ich ehrlich gesagt nicht, wie ich das machen soll (sorry, aber das mit dem 'blutiger Anfänger' war nicht übertrieben ^^)
Python 47 hat geschrieben:zudem:
  • Codezeilen sind zu lang [wiki]Lange Zeilen im Sourcecode[/wiki]
  • Leerzeichen machen den Code übersichtlicher
  • Ofte Codewiederholungen( if, elif) lassen sich besser zusammenfassen
Zu 1. Stimmt, aber irgendwie fand ich es, bevor ich es geändert habe leichter lesbar. Hast du Vorschläge, wie man es trotz kürzerer Zeilen lesbarer halten könnte, als es jetz ist?
zu 2. Habe ich doch? Zumindest in allen Rechnungen. Wo sollte ich noch Leerzeichen machen?
BlackJack hat geschrieben: Wenn der Hauptcode in einer `main()`-Funktion steckt, dann wirst Du feststellen, dass Du an einigen Stellen auf globale Namen zugreifst, anstatt sie als Argumente an die Funktionen zu übergeben. `life` zum Beispiel.
Das verstehe ich ehrlich gesagt nicht? 'life' wird doch immer als Argument an die Funktionen übergeben?
BlackJack

Montag 15. Januar 2007, 19:02

da3dalus hat geschrieben:
Leonidas hat geschrieben:[*]Unflexibler Sheband, besser /usr/bin/env python verwenden
Ok, habe ich gemacht. Aber warum ist das besser?
Weil jetzt das Programm `env` Python aufruft und dabei ist es egal wo Python liegt, solange es im $PATH liegt. Das funktioniert z.B. auch wenn Python unter `/usr/local/bin/python` zu finden ist.
Python 47 hat geschrieben:zudem:
  • Codezeilen sind zu lang [wiki]Lange Zeilen im Sourcecode[/wiki]
  • Leerzeichen machen den Code übersichtlicher
  • Ofte Codewiederholungen( if, elif) lassen sich besser zusammenfassen
Zu 1. Stimmt, aber irgendwie fand ich es, bevor ich es geändert habe leichter lesbar. Hast du Vorschläge, wie man es trotz kürzerer Zeilen lesbarer halten könnte, als es jetz ist?
So wie's jetzt ist, ist die Struktur der Funktion ja auch gar nicht mehr erkennbar. Man sollte die umgebrochenen Zeilen schon so einrücken, dass sie zur Programmstruktur passen.

Ein Hauptproblem ist aber einfach, dass da zu viel Quelltext steht, den man anders und kürzer schreiben sollte.
BlackJack hat geschrieben: Wenn der Hauptcode in einer `main()`-Funktion steckt, dann wirst Du feststellen, dass Du an einigen Stellen auf globale Namen zugreifst, anstatt sie als Argumente an die Funktionen zu übergeben. `life` zum Beispiel.
Das verstehe ich ehrlich gesagt nicht? 'life' wird doch immer als Argument an die Funktionen übergeben?
Ups, dann habe ich das wohl übersehen. :oops:
Python 47
User
Beiträge: 574
Registriert: Samstag 17. September 2005, 21:04

Montag 15. Januar 2007, 20:36

Und auch nochmal ein Ups von mir, ich meinte Leerzeilen, nicht Leerzeichen. :oops:
mfg

Thomas :-)
Benutzeravatar
gerold
Python-Forum Veteran
Beiträge: 5555
Registriert: Samstag 28. Februar 2004, 22:04
Wohnort: Oberhofen im Inntal (Tirol)
Kontaktdaten:

Montag 15. Januar 2007, 22:46

da3dalus hat geschrieben:Hast du Vorschläge, wie man es trotz kürzerer Zeilen lesbarer halten könnte
Hi da3dalus!

Willkommen im Python-Forum!

Hier siehst du, wie ich lange Zeilen kürzer halte:
http://www.python-forum.de/post-40371.html#40371

mfg
Gerold
:-)
http://halvar.at | Kleiner Bascom AVR Kurs
Wissen hat eine wunderbare Eigenschaft: Es verdoppelt sich, wenn man es teilt.
da3dalus
User
Beiträge: 9
Registriert: Sonntag 14. Januar 2007, 20:06

Dienstag 16. Januar 2007, 17:30

Danke für eure Erklärungen und Tips.
Hat einer von euch eine Idee, wie man das ganze sinnvoll mit Objekten schreiben kann? Ich würde eigentlich gerne ein eine Klasse für die Felder machen, aber keine Idee, wie ich das umsetzen soll. Ich will ja nicht jedes Feld/Objekt selber per Hand anlegen müssen (ganz abgesehen davon, dass das auch nicht geht, weil die Größe des Spielfelds ja variabel ist). Kann mir da einer helfen? :D
Benutzeravatar
birkenfeld
Python-Forum Veteran
Beiträge: 1603
Registriert: Montag 20. März 2006, 15:29
Wohnort: Die aufstrebende Universitätsstadt bei München

Dienstag 16. Januar 2007, 18:01

Jedes Feld als eigenes Objekt waer auch etwas Overkill.
Dann lieber noch Vim 7 als Windows 7.

http://pythonic.pocoo.org/
da3dalus
User
Beiträge: 9
Registriert: Sonntag 14. Januar 2007, 20:06

Dienstag 16. Januar 2007, 18:41

birkenfeld hat geschrieben:Jedes Feld als eigenes Objekt waer auch etwas Overkill.
Ok, aber darum geht es mir gar nicht. Auch wenn ich beispielsweise eine Liste pro Zeile haben wollte, müssten die Listen vom Programm erzeugt werden.
Dann würde ich sowas wie
for x in range(1, length):
Zeilex = []
machen wollen, was ja aber nicht geht. Könnt ihr mir sagen, wie man so etwas macht?
BlackJack

Dienstag 16. Januar 2007, 22:40

Wenn Du auf die einzelnen Zeilen über einen Index zugreifen willst, kannst Du sie in einer Liste speichern.

Code: Alles auswählen

rows = list()
for dummy in xrange(length):
    rows.append([0] * length)
Oder als List-Comprehension:

Code: Alles auswählen

rows = [[0] * length for dummy in xrange(length)]
An eine Zelle kommst Du jetzt mit Zeilen und Spaltennummer. Das hier gibt den Wert von Zeile 43 und Spalte 24 aus:

Code: Alles auswählen

print rows[42][23]
da3dalus
User
Beiträge: 9
Registriert: Sonntag 14. Januar 2007, 20:06

Mittwoch 17. Januar 2007, 20:26

BlackJack hat geschrieben:Wenn Du auf die einzelnen Zeilen über einen Index zugreifen willst, kannst Du sie in einer Liste speichern.
Mmmhh, ja, irgendwie habe ich manchmal echt ein Brett vorm Kopf...
Ich habe jetzt nochmal eine neue Variante geschrieben, die mir schon viel besser gefällt, als die erste. Habt ihr dazu noch Anmerkungen?

Code: Alles auswählen

#!/usr/bin/env python
# -*- coding: utf8 -*-
import string
import time
def main():
	length = int(raw_input('Welche Seitenlänge soll das Feld haben? '))
	life = newlife(length)
	answer = None
	while answer != '' and answer != 'nein':
		answer = raw_input('Zelle zum Leben erwecken? Wenn ja, welche?'
		'(Koordinaten): ')
		if answer != 'nein' and answer != '':
			life[int(string.split(answer, ',')[1])]\
			[int(string.split(answer, ',')[0])] = 1
	printlife(life, length)
	mode = int(raw_input('Laufen lassen (1) oder x-te' +
	' Generation zeigen? '))
	if mode == 1:
		while True:
			printlife(life, length)
			print
			life = evolve(life, length)
			time.sleep(1)
	else: 
		for i in xrange(1, mode):
			life = evolve(life, length)
		printlife(life, length)

def printlife(life, length):
	for y in xrange(1, length + 1):
		for x in xrange(1, length + 1):
			if life[y][x] == 0:
				print ' ',
			else: print 'o',
		print

def evolve(life, length):
	oldgeneration = life
	life = newlife(length)
	for y in xrange(1, length + 1):
		for x in xrange(1, length + 1):
			if neighbours(oldgeneration, y, x) < 2 or \
			neighbours(oldgeneration, y, x) > 3 or \
			neighbours(oldgeneration, y, x) == 2 and \
			oldgeneration[y][x] == 0:
				life[y][x] = 0
			else:
				life[y][x] = 1
			
	return life

def newlife(length):
	life = []
	for i in xrange(0, length + 2):
		life.append([])
		for z in xrange(0, length + 2):
			life[i].append(0)
	return life

def neighbours(life, y, x):
	neighbours = (
	life[y + 1][x - 1] + life[y + 1][x] + life[y + 1][x + 1] +
	life[y][x - 1] + life[y][x + 1] +
	life[y - 1][x - 1] + life[y - 1][x] + life[y - 1][x + 1])
	return neighbours
main()
Das mit den Docstrings und den nicht PEP8-konformen Namen habe ich noch im Hinterkopf, darüber muss ich mich aber erst noch belesen.
BlackJack

Mittwoch 17. Januar 2007, 23:04

Aus der Eingabeschleife für die Koordinaten könnte man eine "Endlosschleife" machen, die abgebrochen wird wenn '' oder 'nein' eingegeben wurde. Damit spart man die Zuweisung von `None` an `answer` vor der Schleife und muss den Test nur an einer Stelle machen.

Das aufsplitten und umwandeln der Koordinaten wird zweimal gemacht und führt zu einem ziemlich langen Ausdruck den Du auf zwei Zeilen aufgeteilt hast. Wenn sowieso zwei Zeilen benötigt werden, kann man das auch effizienter und lesbarer machen.

Code: Alles auswählen

    while True:
        answer = raw_input('Zelle zum Leben erwecken? Wenn ja, welche?'
                           ' (Koordinaten): ')
        if answer in ('', 'nein'):
            break
        x, y = map(int, string.split(answer, ','))
        life[y][x] = True
Da jede Zelle den Wert 0 oder 1 enthält und auf einzelne Zeichen in Zeichenketten über einen Index zugegriffen werden kann, lässt sich die Ausgabe einer Zelle so lösen:

Code: Alles auswählen

def printlife(life, length):
    for y in xrange(1, length + 1):
        for x in xrange(1, length + 1):
            print ' o'[life[y][x]],
        print
Die Berechnung der Anzahl der Nachbarn sollte man in `evolve()` aus der ``if`-Abfrage herausziehen, die braucht nur *einmal* ausgeführt werden:

Code: Alles auswählen

    for y in xrange(1, length + 1):
        for x in xrange(1, length + 1):
            n = neighbours(oldgeneration, y, x)
            life[y][x] = 2 <= n <= 3 and (n != 2 or oldgeneration[y][x]) 
`newlife()` lässt sich mit einer List-Comprehension und der Multiplikation auf Listen wesentlich kürzer schreiben:

Code: Alles auswählen

def newlife(length): 
    return [[False] * (length + 2) for dummy in xrange(length + 2)]
Beim zählen der Nachbarn kann man die relativen Koordinaten herausziehen. Verringert die Tipparbeit und macht die Funktion übersichtlicher.

Code: Alles auswählen

def neighbours(life, y, x):
    offsets = ((1, -1), (1, 0), (1, 1), (0, -1),
               (0, 1), (-1, -1), (-1, 0), (-1, 1))
    return sum(life[y + dy][x + dx] for dy, dx in offsets)
da3dalus
User
Beiträge: 9
Registriert: Sonntag 14. Januar 2007, 20:06

Donnerstag 18. Januar 2007, 18:16

BlackJack hat geschrieben: Da jede Zelle den Wert 0 oder 1 enthält und auf einzelne Zeichen in Zeichenketten über einen Index zugegriffen werden kann, lässt sich die Ausgabe einer Zelle so lösen:

Code: Alles auswählen

def printlife(life, length):
    for y in xrange(1, length + 1):
        for x in xrange(1, length + 1):
            print ' o'[life[y][x]],
        print
Alle anderen Vorschläge finde ich super (und nachdem ich mich schnell über map() und list-compehension belesene habe) auch verständlich, aber hier sehe ich ehrlich gesagt den Vorteil nicht. Ok, man spart 2 Zeilen Code, aber dafür ist das andere meiner Meinung nach verständlicher.
Antworten