Review people #11

Fred · 2020-10-20T15:56:54+02:00

Fred commented

2020-10-20 15:56:54 +02:00

Est-ce qu'il ne faudrait pas refactoriser les GSM père/mère ? Avec une table d'association, et un champ qui indique la qualité du contact ? Du coup, cela permettrait se débarasser des champs phone, gsm, gsm_father et gsm_mother.
Comment sont gérées les évolutions ? Changement de clubs, de fédération,
A quoi correspond le champ year_of_practice ? Comment se comportera-t-il dans un an ? Dans deux ans ? Est-ce qu'il ne faudrait pas une date de début, plutôt ? Idem pour la méthode actual_year_of_pratice. Comme elle se base sur le champ created_at, il suffit que l'enregistrement ne soit pas réalisé correctement pour que la méthode retourne une mauvaise information.
Que signifie qu'un gymnaste soit actif ou inactif ? Est-ce que cela ne devrait pas plutôt être géré au niveau des utilisateurs ?
Au niveau des utilisateurs, comme un User Django a déjà les champs lastname/firstname, pourquoi ne pas les réutiliser ? On garde la clé OneToOne, mais on déplace dans l'autre classe les champs qui peuvent l'être. Email s'y retrouve également.
Les relations cando, haveToDo et have_routine ne sont pas correctement nommées. Si tu as une instance de Gymnast, tu devrais faire ceci :

>>> gregg = Gymnast.objects.first()
>>> gregg.have_routine <-- pas bien
>>> gregg.routines <-- ok

Idéalement, cela pourrait même être une indirection.

>>> gregg.routines <-- retourne la relation de type `have_routine`
>>> gregg.educatives.can_do <-- retourne les éducatifs qu'il **peut** faire
>>> gregg.educatives.must_do <-- retourne les éducatifs qu'il **doit** faire

(j'avoue ne pas tout à fait comprendre la nuance entre les deux)

<!> Tu as des fonctions qui ne sont pas du tout utilisées et qui pourrissent un peu le fichier.

next_age() ? N'est appelé nulle part ailleurs.
known_skills() <-- peut être récupéré directement via l'attribut cando
(du coup, tu as sans doute une piste de renommage ;-))

* Est-ce qu'il ne faudrait pas refactoriser les GSM père/mère ? Avec une table d'association, et un champ qui indique la qualité du contact ? Du coup, cela permettrait se débarasser des champs phone, gsm, gsm_father et gsm_mother. * Comment sont gérées les évolutions ? Changement de clubs, de fédération, * A quoi correspond le champ `year_of_practice` ? Comment se comportera-t-il dans un an ? Dans deux ans ? Est-ce qu'il ne faudrait pas une date de début, plutôt ? Idem pour la méthode `actual_year_of_pratice`. Comme elle se base sur le champ `created_at`, il suffit que l'enregistrement ne soit pas réalisé correctement pour que la méthode retourne une mauvaise information. * Que signifie qu'un gymnaste soit actif ou inactif ? Est-ce que cela ne devrait pas plutôt être géré au niveau des utilisateurs ? * Au niveau des utilisateurs, comme un User Django a déjà les champs lastname/firstname, pourquoi ne pas les réutiliser ? On garde la clé OneToOne, mais on déplace dans l'autre classe les champs qui peuvent l'être. `Email` s'y retrouve également. * Les relations `cando`, `haveToDo` et `have_routine` ne sont pas correctement nommées. Si tu as une instance de `Gymnast`, tu devrais faire ceci : ```python >>> gregg = Gymnast.objects.first() >>> gregg.have_routine <-- pas bien >>> gregg.routines <-- ok ``` Idéalement, cela pourrait même être une indirection. ```python >>> gregg.routines <-- retourne la relation de type `have_routine` >>> gregg.educatives.can_do <-- retourne les éducatifs qu'il **peut** faire >>> gregg.educatives.must_do <-- retourne les éducatifs qu'il **doit** faire ``` (j'avoue ne pas tout à fait comprendre la nuance entre les deux) * <!> Tu as des fonctions qui ne sont pas du tout utilisées et qui pourrissent un peu le fichier. >>> next_age() ? N'est appelé nulle part ailleurs. >>> known_skills() <-- peut être récupéré directement via l'attribut `cando` (du coup, tu as sans doute une piste de renommage ;-))

Sulley changed title from ~~[WIP] review/people~~ to Review people

2020-10-26 11:06:54 +01:00

Sulley closed this pull request

2020-10-26 11:07:03 +01:00

Sign in to join this conversation.

No reviewers