[Location] Statistiques d'un club #44

New Issue

Fred · 2020-10-29T20:52:42+01:00

Fred commented

2020-10-29 20:52:42 +01:00

Je vais surtout aborder la fonction club_statistics, dans la mesure où elle fait pratiquement 150 lignes (soit, 10x plus que ce que recommanderait Roel :-p).

Modèle

courses = Course.objects.filter(club__in=clubid).order_by(
        "iso_day_number", "hour_begin"
    )

<- ce passage-ci fait penser qu'il manque un related_name entre Club et Course, et un order_by par défaut au niveau de la classe Course.

Nomenclature

De manière générale, dans la fonction, essaie de respecter la nomenclature:

courseList -> courses_list
totalHoursPaidForCourse -> total_hours_paid (et comme on est dans le contexte du cours, je ne pense pas qu'il soit utile de spécifier le suffixe)
totalCourses -> number_of_courses
...

Un autre point concerne la sémantique:

...
totalHoursByWeek += nbhour.seconds
...
totalHoursByWeek = totalHoursByWeek / 3600

Donc, l'idée est d'ajouter des secondes à une variable qui s'appelle totalHoursByWeek.
Tout à la fin, tu divises la valeur obtenue par 3600.
On a tous voté dans ma tête, et c'est non.

Structures

Tu as un dictionaire qui reprend la liste des gymnastes gymnastsDict = {}.

Pour chaque cours, tu as l'air de construire un ensemble de gymnastes:

list_of_gymnasts = Gymnast.objects.filter(to_gym__in=course.to_subgroup.all())
gymnasts.extend(list_of_gymnasts)
nbgymnast = len(list_of_gymnasts)

Le gros bloc ci-dessous laisse penser que tu as besoin d'une structure, avec des attributs par défaut:

if gymnast.id not in gymnastsDict:
    gymnastsDict[gymnast.id] = {
        "gymnast": gymnast,
        "nbcoursebyweek": 0,
        "nbhourbyweek": timedelta(),
        "nbtraining": 0,
        "nbattendance": 0,
        "nbabsence": 0,
        "nbhourtraining": 0,
        "nbhourattendance": timedelta(),
        "percentageattendance": 0,
        "nbhourabsence": 0,
        "percentageabsence": 0,
    }

->

class GymnastStatistics:
	def __init__(self, gymnast):
    	self.gymnast = gymnast
        self.number_of_courses_by_week = 0
        self.number_of_trainings = 0
        self.number_of_attendances = 0
        ...

Tu as aussi un bloc (lignes 235-236):

attendanceList = Training.objects.filter(course=course, gymnast=gymnast)
nbattendance = len(attendanceList)

Mais en gros, tu n'utilises après que la variable nbattendance.
Autant faire directement un count sur le queryset, sans quoi la liste complète est évaluée à chaque tour de boucle. Le résultat peut sans doute être passé directement au constructeur de la classe GymnastStatistics dont il est question ci-dessus.

Ensuite, il suffit de créer une méthode add_course sur la classe ci-dessus pour gérer toutes les lignes suivantes:

gymnastsDict[gymnast.id]["nbcoursebyweek"] += 1
gymnastsDict[gymnast.id]["nbhourbyweek"] += nbhour  # timedelta
gymnastsDict[gymnast.id]["nbtraining"] += counted
gymnastsDict[gymnast.id]["nbattendance"] += nbattendance
gymnastsDict[gymnast.id]["nbhourtraining"] += totalHourForCourse
gymnastsDict[gymnast.id]["nbhourattendance"] += (
	nbhour * nbattendance
)

Cela évitera aussi de jongler avec les clés du dictionnaire, puisque ce seront de "vrais" attributs définis au niveau de la classe.

Structures ²

Le dernier passage, ce sont les lignes 257-292. Tu peux les simplifier en ajoutant toutes ces informations au niveau de la classe GymnastStatistics, avec un mécanisme de getter/setter si tu le juges nécessaire, ou simplement via une méthode export ou to_dict ou ... qui renverrait ces données, calculées sur base des attributs de la classe.

Typiquement:

gymnastsDict[gymnast.id]["percentageattendance"] = int(
(
	gymnastsDict[gymnast.id]["nbhourattendance"]
	/ gymnastsDict[gymnast.id]["nbhourtraining"]
)
* 100
)

pourra se résumer avec une méthode (ou un getter) type

def percentage_of_attendance(self):
	return int(self.number_of_attendance / self.number_hours_of_training) * 100

qui sera un chouia plus lisible ;-)

Refactoring

De la ligne 177 à la ligne 204, tu te trouves dans une boucle, où, pour chaque cours, tu calcules un ensemble d'informations.

Sors ces données et fais-en une méthode du modèle Course. Cela allégera énormément la lecture et simplifiera également la gestion du code.

Je vais surtout aborder la fonction `club_statistics`, dans la mesure où elle fait pratiquement 150 lignes (soit, 10x plus que ce que recommanderait Roel :-p). ## Modèle ```python courses = Course.objects.filter(club__in=clubid).order_by( "iso_day_number", "hour_begin" ) ``` <- ce passage-ci fait penser qu'il manque un `related_name` entre `Club` et `Course`, et un `order_by` par défaut au niveau de la classe `Course`. ## Nomenclature De manière générale, dans la fonction, essaie de respecter la nomenclature: * `courseList` -> `courses_list` * `totalHoursPaidForCourse` -> `total_hours_paid` (et comme on est dans le contexte du cours, je ne pense pas qu'il soit utile de spécifier le suffixe) * `totalCourses` -> `number_of_courses` * ... Un autre point concerne la sémantique: ```python ... totalHoursByWeek += nbhour.seconds ... totalHoursByWeek = totalHoursByWeek / 3600 ``` Donc, l'idée est d'ajouter des secondes à une variable qui s'appelle `totalHoursByWeek`. Tout à la fin, tu divises la valeur obtenue par 3600. On a tous voté dans ma tête, et c'est non. ## Structures Tu as un dictionaire qui reprend la liste des gymnastes `gymnastsDict = {}`. Pour chaque cours, tu as l'air de construire un ensemble de gymnastes: ```python list_of_gymnasts = Gymnast.objects.filter(to_gym__in=course.to_subgroup.all()) gymnasts.extend(list_of_gymnasts) nbgymnast = len(list_of_gymnasts) ``` Le gros bloc ci-dessous laisse penser que tu as besoin d'une structure, avec des attributs par défaut: ```python if gymnast.id not in gymnastsDict: gymnastsDict[gymnast.id] = { "gymnast": gymnast, "nbcoursebyweek": 0, "nbhourbyweek": timedelta(), "nbtraining": 0, "nbattendance": 0, "nbabsence": 0, "nbhourtraining": 0, "nbhourattendance": timedelta(), "percentageattendance": 0, "nbhourabsence": 0, "percentageabsence": 0, } ``` -> ```python class GymnastStatistics: def __init__(self, gymnast): self.gymnast = gymnast self.number_of_courses_by_week = 0 self.number_of_trainings = 0 self.number_of_attendances = 0 ... ``` Tu as aussi un bloc (lignes 235-236): ```python attendanceList = Training.objects.filter(course=course, gymnast=gymnast) nbattendance = len(attendanceList) ``` Mais en gros, tu n'utilises après que la variable `nbattendance`. Autant faire directement un `count` sur le queryset, sans quoi la liste complète est évaluée à chaque tour de boucle. Le résultat peut sans doute être passé directement au constructeur de la classe `GymnastStatistics` dont il est question ci-dessus. Ensuite, il suffit de créer une méthode `add_course` sur la classe ci-dessus pour gérer toutes les lignes suivantes: ```python gymnastsDict[gymnast.id]["nbcoursebyweek"] += 1 gymnastsDict[gymnast.id]["nbhourbyweek"] += nbhour # timedelta gymnastsDict[gymnast.id]["nbtraining"] += counted gymnastsDict[gymnast.id]["nbattendance"] += nbattendance gymnastsDict[gymnast.id]["nbhourtraining"] += totalHourForCourse gymnastsDict[gymnast.id]["nbhourattendance"] += ( nbhour * nbattendance ) ``` Cela évitera aussi de jongler avec les clés du dictionnaire, puisque ce seront de "vrais" attributs définis au niveau de la classe. ## Structures ² Le dernier passage, ce sont les lignes 257-292. Tu peux les simplifier en ajoutant toutes ces informations au niveau de la classe `GymnastStatistics`, avec un mécanisme de getter/setter si tu le juges nécessaire, ou simplement via une méthode `export` ou `to_dict` ou ... qui renverrait ces données, calculées sur base des attributs de la classe. Typiquement: ```python gymnastsDict[gymnast.id]["percentageattendance"] = int( ( gymnastsDict[gymnast.id]["nbhourattendance"] / gymnastsDict[gymnast.id]["nbhourtraining"] ) * 100 ) ``` pourra se résumer avec une méthode (ou un getter) type ```python def percentage_of_attendance(self): return int(self.number_of_attendance / self.number_hours_of_training) * 100 ``` qui sera un chouia plus lisible ;-) ## Refactoring De la ligne 177 à la ligne 204, tu te trouves dans une boucle, où, pour chaque cours, tu calcules un ensemble d'informations. Sors ces données et fais-en une méthode du modèle `Course`. Cela allégera énormément la lecture et simplifiera également la gestion du code.

Fred added this to the Revue de l'application `location` milestone 2020-10-29 20:52:42 +01:00

Sulley was assigned by Fred

2020-10-29 20:52:42 +01:00

Sulley commented

2020-11-01 16:51:11 +01:00

Waouw… Tu penses qu'il y aurait moyen de découper ce ticket en plusieurs "sous-ticket"s ? Parce que la c'est long… Mais je vais commencer à y travailler dessus.