@Redstonerayy: Der Fehler kommt dadurch zustande das die `make_groups()`-Funktion implizit auch `None` zurückgeben kann und `None` hat halt keine Länge. Die Funktion geht ja in den letzten ``else``-Zweig wie man an der Ausgabe sehen kann, und da steht nirgends ein ``return`` das etwas zurück gibt mit der Code der die Funktion aufruft etwas anfangen kann, denn der will auf jeden Fall etwas das eine Länge hat.
Code: Alles auswählen
In [5]: len(None)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-5-96b07938784c> in <module>
----> 1 len(None)
TypeError: object of type 'NoneType' has no len()
Die Funktion sollte an der Stelle am besten keine Ausgabe per `print()` machen sondern eine Ausnahme auslösen, und die entsprechende Behandlung und Ausgabe für den Benutzer dann an der Stelle passieren wo die Funktion aufgerufen wird.
Weitere Anmerkungen zum Code: Der erste Kommentar wäre eine gute Grundlage für den Docstring des Moduls.
Kommentare sollten einen Mehrwert bieten. Über den Importen zu ``# Imports`` zu kommentieren bringt dem Leser keine nützliche Information. Faustregel: Kommentare sollten nicht beschreiben *was* passiert, denn das steht da bereits als Code, sondern warum der Code das *so* macht. Sofern das nicht offensichtlich ist. Bevor man die Bedeutung von Namen kommentiert, sollte man schauen ob man nicht einen besseren Namen findet, so dass man den nicht erklären muss.
Sternchen-Importe machen Programme unübersichtlicher, weil man als Leser nicht immer alle Namen kennt die damit definiert werden. Es sind ja auch nicht nur die Namen die für das Modul dokumentiert sind und die in dem Modul definiert werden, sondern auch Namen die das Modul selbst irgendwo anders her importiert. Ich sehe auf Anhieb nicht welche Funktionen aus `random` verwendet werden, dazu muss ich das gesamte Programm absuchen und *wissen* wonach ich suche.
Konkrete Grunddatentypen haben in Namen nichts zu suchen. Das ändert sich öfter mal während der Programmentwicklung, sei es weil man einen passenderen Typ aus der Standardbibliothek oder externen Paketen verwendet, oder weil man sich irgendann selbst einen spezifischeren Typ für etwas schreibt was am Anfang mal eine Liste, ein Wörterbuch, oder ähnliches war. Da muss man dann entweder das ganze Programm absuchen und alle betroffenen Namen ändern, oder man hat irreführende Namen im Programm. `local_list` ist ein besonders nichtssagender Name. Beide Informationen die in dem Namen stehen sind ganz offensichtlich am Code ablesbar, egal wie der Name lauten würde. Das ist ein lokaler Name — wird als erstes in der Funktion definiert — und es wird bei der Definition eine Liste an den Namen gebunden. Was da fehlt wäre die Information was denn in dieser Liste gespeichert — was für eine Bedeutung sie im Code hat.
Konstanten schreibt man in Python komplett in Grossbuchstaben.
Auf Modulebene sollte nur Code stehen der Konstanten, Funktionen, und Klassen definiert. Das Hauptprogramm steht üblicherweise in einer Funktion die `main()` heisst.
Namen sollte man nicht nummerieren. Besonders unsinnig ist es einfach irgendwo eine 1 an einen Namen zu hängen wenn es noch nicht einmal den Namen mit einer 2 gibt. Wenn man Namen durchnummeriert will man sich entweder bessere Namen ausdenken, oder gar keine Einzelnamen vergeben sondern eine Datenstruktur verwenden. Oft eine Liste.
`_arg` an Argumentnamen anzuhängen macht auch keinen Sinn. Das sieht der Leser doch welche Namen Argumente sind. Funktionen sollten nicht so lang werden dass man das nicht mehr leicht ablesen kann.
`make_groups()` könnte ein bisschen besser strukturiert sein. Die Bedingung die zu der Fehlermeldung führt, und die eigentlich zu einer Ausnahme führen sollte, kann man gleich ganz an den Anfang ziehen, denn man braucht davor gar erst *irgendwas* zu machen, wenn die Aufgabe der Funktion mit den gegebenen Argumenten gar nicht erfüllt werden kann. Damit spart man sich dann auch gleich eine Einrückungsebene für den Rest der Funktion.
`local_list` wird zu früh definiert. Das sollte *in* der ``while``-Schleife am *Anfang* stehen. Dann braucht diese gleiche Zuweisung einer leeren Liste auch nicht *zweimal* im Code stehen haben. Das der Name schlecht ist, schrob ich ja weiter oben schon. Da werden die Namen einer Gruppe gesammelt, also wäre `group` ein sinnvoller Name dafür.
Zwei Namen `number_of_people` und `number_of_remaining_people` für die gleiche Information zu verwenden ist ein Name zu viel. Eigentlich braucht man dafür gar keinen Namen, da Du ja Namen aus der Liste entfernst, also `number_of_remaining_people` immer den gleichen Wert hat wie die Länge der Liste. Dann kann man auch die Länge der Liste abfragen statt die ausserhalb der Liste noch mal parallel mit zu zählen.
Wenn man aus syntaktischen Gründen einen Namen braucht, den Namen aber nicht verwendet, gibt es die Konvention `_` als Namen zu benutzen.
Die `make_groups()`-Funktion könnte dann so aussehen:
Code: Alles auswählen
def make_groups(group_size, names):
"""
Take the group size and the names and return a list with the groups.
"""
if len(names) < group_size:
raise ValueError(
f"There are not enough names to make groups."
f" Here are all names: {names}"
)
groups = []
while len(names) >= group_size:
group = []
for _ in range(group_size):
group.append(random.choice(names))
names.remove(group[-1])
groups.append(group)
return groups
Was daran äusserst unschön ist, ist das die Liste mit den Namen verändert wird. Funktion sollten nicht solche Nebeneffekte haben, und wenn, dann sollte man die *deutlich* dokumentieren, denn damit rechnet der Leser nicht. Man könnte hier beispielsweise am Anfang der Funktion eine Kopie der Liste anlegen, was zusätzlich den Vorteil hätte, das die Funktion nicht nur mit einer Namens*liste* klar käme, sondern mit jedem beliebigen endlichen iterierbaren Objekt. Und da hätte wir dann auch schon ein Beispiel wo `list_of_names` oder `names_list` plötzlich falsch und irreführend als Argumentname wäre wenn man den Code durch eine einzige Zeile so ändert das es nicht zwingend eine Liste sein muss.
Wenn man die Namen in einer lokalen Liste sammelt, kann man das ganze auch noch viel effizienter schreiben als immer einzelne Namen aus der Liste auszuwählen und aus der einen Liste entfernen um sie in eine andere Liste zu stecken. Man kann einfach die Namen zufällig mischen und dann die Ergebnislisten per „slicing“ erstellen.
Code: Alles auswählen
def make_groups(group_size, names):
"""
Take the group size and the names and return a list with the groups.
"""
names = list(names)
group_count = len(names) // group_size
if group_count == 0:
raise ValueError(
f"There are not enough names to make groups."
f" Here are all names: {names}"
)
random.shuffle(names)
return [
names[i * group_size : i * group_size + group_size]
for i in range(group_count)
]
Man könnte sich auch überlegen ob man den Sonderfall mit keiner Gruppe überhaupt so ”besonders” behandeln muss, und in dem Fall nicht einfach eine leere Liste zurück gibt, was ja nicht einmal extra Code benötigen würde.
Auch in der Hauptfunktion könnte der Code besser strukturiert sein. Das erste ``if`` gehört eigentlich *in* den Fall das man schon weiss, dass der Benutzer "groups" eingegeben hat, denn nur dann benötigt man die Umwandlund des zweiten Elements in eine ganze Zahl. Die sollte man dann aber nicht mehr in die Liste zurück speichern. In Listen sollte man Objekte speichern die alle den gleichen (Duck-)Typ haben, nicht Zeichenketten und Zahlen mischen.
Der Kommentar bei dem ``if`` ist auch falsch, denn da wird nicht geprüft ob der Benutzer "groups" eingegeben hat, sondern ob er genau ein Komma in der Eingabe hatte.
``continue`` sollte man vermeiden. Das ist ein unbedingter Sprung an den Schleifenanfang den man nicht an der Einrückung des Quellcodes erkennen kann. Das kann sehr schnell sehr unübersichtlich werden.
Das hantieren mit ”magischen” Indexwerten ist nicht schön. Statt also immer mit 0 und 1 auf die gesplittete Eingabe zuzugreifen, sollte man die Teile an Namen binden. Also mindestens mal den Wert an Index 0, denn das ist ja immer der Name des Kommandos das ausgeführt werden soll.
Literale Zeichenketten sind nicht dazu da um Code auszukommentieren.
``for i in range(len(sequence)):`` ist in Python ein „anti-pattern“. Man kann direkt über die Elemente von Sequenztypen wie Listen iterieren, ohne den Umweg über einen Laufindex.
Zwischenstand:
Code: Alles auswählen
#!/usr/bin/env python3
"""
This Program allows to chose a random person from a list or to make random
groups.
"""
import random
NAMES = [
"Adrien",
"Anna",
"Arden",
"Camilla",
"Dario",
"Darleen",
"Fleta",
"Inez",
"Jen",
"Johanna",
"Martha",
"Megan",
"Peg",
"Roselia",
"Sibyl",
"Sixta",
"Stefanie",
"Sun",
"Tobi",
"Tyson",
]
def make_groups(group_size, names):
"""
Take the group size and the names and return a list with the groups.
"""
names = list(names)
random.shuffle(names)
return [
names[i * group_size : i * group_size + group_size]
for i in range(len(names) // group_size)
]
def main():
while True:
command, *arguments = (
input(
"Enter groups and comma separated the size of them with or person\n"
"if you want the Program to make random groups or chose a random person!\n"
"(groups,(number)/person/quit): "
)
.lower()
.split(",")
)
if command == "groups":
try:
group_size = int(arguments[0])
except (IndexError, ValueError):
print("Need a number as argument!")
else:
groups = make_groups(group_size, NAMES)
if groups:
print("Here are the groups")
for group in groups:
print(group)
else:
print(
f"There where not enough names for given"
f" group size {group_size}."
)
elif command == "person":
print(random.choice(NAMES))
elif command == "quit":
break
else:
print(f"Unknown command {command!r}")
if __name__ == "__main__":
main()
