objective/views.py - routine_details #20

New Issue

Fred · 2022-01-08T22:28:20+01:00

Fred commented

2022-01-08 22:28:20 +01:00

Yop,

Je me suis un peu étouffé en lisant ce morceau de code:

def routine_details(request, routineid):
    """
    Récupère toutes les informations d'une routine (série).

    :param routineid: id d'une `routine`
    :type routineid: int
    :return: routineid
    """

    routine = get_object_or_404(Routine, pk=routineid)

    rank = 0
    level = 0
    age_boy_with_help = 0
    age_girl_with_help = 0
    age_boy_without_help = 0
    age_girl_without_help = 0
    age_boy_chained = 0
    age_girl_chained = 0
    age_boy_masterised = 0
    age_girl_masterised = 0
    difficulty = 0
    is_competitive = True

    for skill_link in routine.skill_links.all():

        skill_link.skill
        current_skill

        difficulty += skill_link.skill.difficulty
        level = max(skill_link.skill.level, level)
        rank = max(skill_link.skill.rank + 1, rank)

        if not skill_link.skill.is_competitive:
            is_competitive = False

        # Age boy computing
        if skill_link.skill.age_boy_with_help is not None and skill_link.skill.age_boy_with_help > age_boy_with_help:
            age_boy_with_help = skill_link.skill.age_boy_with_help

        if skill_link.skill.age_boy_without_help is not None and skill_link.skill.age_boy_without_help > age_boy_without_help:
            age_boy_without_help = skill_link.skill.age_boy_without_help

        if skill_link.skill.age_boy_chained is not None and skill_link.skill.age_boy_chained > age_boy_chained:
            age_boy_chained = skill_link.skill.age_boy_chained

        if skill_link.skill.age_boy_masterised is not None and skill_link.skill.age_boy_masterised > age_boy_masterised:
            age_boy_masterised = skill_link.skill.age_boy_masterised

        # Age girl computing
        if skill_link.skill.age_girl_with_help is not None and skill_link.skill.age_girl_with_help > age_girl_with_help:
            age_girl_with_help = skill_link.skill.age_girl_with_help

        if skill_link.skill.age_girl_without_help is not None and skill_link.skill.age_girl_without_help > age_girl_without_help:
            age_girl_without_help = skill_link.skill.age_girl_without_help

        if skill_link.skill.age_girl_chained is not None and skill_link.skill.age_girl_chained > age_girl_chained:
            age_girl_chained = skill_link.skill.age_girl_chained

        if skill_link.skill.age_girl_masterised is not None and skill_link.skill.age_girl_masterised > age_girl_masterised:
            age_girl_masterised = skill_link.skill.age_girl_masterised

    if routine.skill_links.all().count() != 10:
        is_competitive = False

    routine.difficulty = difficulty
    routine.level = max(routine.level, level)
    routine.rank = max(routine.rank, rank)

    routine.age_boy_with_help = max(routine.age_boy_with_help, age_boy_with_help)
    routine.age_boy_without_help = max(
        routine.age_boy_without_help,
        age_boy_without_help
    )
    routine.age_boy_chained = max(routine.age_boy_chained, age_boy_chained)
    routine.age_boy_masterised = max(routine.age_boy_masterised, age_boy_masterised)

    routine.age_girl_with_help = max(routine.age_girl_with_help, age_girl_with_help)
    routine.age_girl_without_help = max(
        routine.age_girl_without_help,
        age_girl_without_help
    )
    routine.age_girl_chained = max(routine.age_girl_chained, age_girl_chained)
    routine.age_girl_masterised = max(routine.age_girl_masterised, age_girl_masterised)

    routine.is_competitive = is_competitive

    routine.save()

    context = {"routine": routine, "skill_link_list": routine.skill_links.all()}
    return render(request, "objectives/routines/details.html", context)

(Si, si, je t'ai vu : edfb858aa9)

En gros, en lisant la fonction, elle ne fait finalement que deux choses:

Mettre à jour les détails de la routine, en allant piocher plein d'informations parmi les autres concepts auxquels elle est liée
Construire le contexte à afficher, qui ne contient que deux données: la routine 👍 et la liste des compétences auxquelles elle est liée.

En fait, tout se qui se trouve entre le get_object_or_404 et le context = {...} doit se trouver ailleurs que dans cette fonction.
C'est impossible à tester ou vérifier de manière automatique si cela se trouve dans une vue et cela viole tous les principes de structuration logique.
Copie/colle le directement dans une méthode du modèle que tu pourrais appeler rebuild que cela fonctionnerait parfaitement 😉

Une deuxième amélioration possible concerne la lisibilité du bloc: comme tu appelles à chaque fois 28 fois (si, j'ai compté 😛 'fin pas moi tout seul...) skill_link.skill, rien que ceci pourrait être renommé en skill ou current_skill que tu gagnerais 11 caractères (x28).

Ensuite, tu as un pattern qui se répète :

if <skill>.<property> is not None and <skill>.<property> > <property_aussi>:
            <property_aussi> = <skill>.<property>

Si cela se répète autant, mets le dans une fonction.
Un truc tout bête hein, mais un truc quand même:

def max_even_if_none(value1, value2):
	if value1 is not None and value 1 > value2:
    	return value1
       
    return value2

Rien qu'avec ceci, ton code pourrait ressembler à ça:

for skill_link in routine.skill_links.all():

        skill = skill_link.skill

        difficulty += skill.difficulty
        level = max(skill.level, level)
        rank = max(skill.rank + 1, rank)

        if not skill.is_competitive:
            is_competitive = False

        age_boy_with_help = max_even_if_none(skill.age_boy_with_help, age_boy_with_help)
        age_boy_without_help = max_even_if_none(skill.age_boy_without_help, age_boy_without_help)
        age_boy_chained = max_even_if_none(skill.age_boy_chained, age_boy_chained)
        age_boy_masterised = max_even_if_none(skill.age_boy_masterised, age_boy_masterised)

        age_girl_with_help = max_even_if_none(skill.age_girl_with_help, age_girl_with_help)
        age_girl_without_help = max_even_if_none(skill.age_girl_without_help, age_girl_without_help)
        age_girl_chained = max_even_if_none(skill.age_girl_chained, age_girl_chained)
        age_girl_masterised = max_even_if_none(skill.age_girl_masterised, age_girl_masterised)

Yop, Je me suis un peu étouffé en lisant ce morceau de code: ```python def routine_details(request, routineid): """ Récupère toutes les informations d'une routine (série). :param routineid: id d'une `routine` :type routineid: int :return: routineid """ routine = get_object_or_404(Routine, pk=routineid) rank = 0 level = 0 age_boy_with_help = 0 age_girl_with_help = 0 age_boy_without_help = 0 age_girl_without_help = 0 age_boy_chained = 0 age_girl_chained = 0 age_boy_masterised = 0 age_girl_masterised = 0 difficulty = 0 is_competitive = True for skill_link in routine.skill_links.all(): skill_link.skill current_skill difficulty += skill_link.skill.difficulty level = max(skill_link.skill.level, level) rank = max(skill_link.skill.rank + 1, rank) if not skill_link.skill.is_competitive: is_competitive = False # Age boy computing if skill_link.skill.age_boy_with_help is not None and skill_link.skill.age_boy_with_help > age_boy_with_help: age_boy_with_help = skill_link.skill.age_boy_with_help if skill_link.skill.age_boy_without_help is not None and skill_link.skill.age_boy_without_help > age_boy_without_help: age_boy_without_help = skill_link.skill.age_boy_without_help if skill_link.skill.age_boy_chained is not None and skill_link.skill.age_boy_chained > age_boy_chained: age_boy_chained = skill_link.skill.age_boy_chained if skill_link.skill.age_boy_masterised is not None and skill_link.skill.age_boy_masterised > age_boy_masterised: age_boy_masterised = skill_link.skill.age_boy_masterised # Age girl computing if skill_link.skill.age_girl_with_help is not None and skill_link.skill.age_girl_with_help > age_girl_with_help: age_girl_with_help = skill_link.skill.age_girl_with_help if skill_link.skill.age_girl_without_help is not None and skill_link.skill.age_girl_without_help > age_girl_without_help: age_girl_without_help = skill_link.skill.age_girl_without_help if skill_link.skill.age_girl_chained is not None and skill_link.skill.age_girl_chained > age_girl_chained: age_girl_chained = skill_link.skill.age_girl_chained if skill_link.skill.age_girl_masterised is not None and skill_link.skill.age_girl_masterised > age_girl_masterised: age_girl_masterised = skill_link.skill.age_girl_masterised if routine.skill_links.all().count() != 10: is_competitive = False routine.difficulty = difficulty routine.level = max(routine.level, level) routine.rank = max(routine.rank, rank) routine.age_boy_with_help = max(routine.age_boy_with_help, age_boy_with_help) routine.age_boy_without_help = max( routine.age_boy_without_help, age_boy_without_help ) routine.age_boy_chained = max(routine.age_boy_chained, age_boy_chained) routine.age_boy_masterised = max(routine.age_boy_masterised, age_boy_masterised) routine.age_girl_with_help = max(routine.age_girl_with_help, age_girl_with_help) routine.age_girl_without_help = max( routine.age_girl_without_help, age_girl_without_help ) routine.age_girl_chained = max(routine.age_girl_chained, age_girl_chained) routine.age_girl_masterised = max(routine.age_girl_masterised, age_girl_masterised) routine.is_competitive = is_competitive routine.save() context = {"routine": routine, "skill_link_list": routine.skill_links.all()} return render(request, "objectives/routines/details.html", context) ``` (Si, si, je t'ai vu : https://sources.grimbox.be/Sulley/Ultron/commit/edfb858aa990582a89295b8b60576ca80a03ed70) En gros, en lisant la fonction, elle ne fait finalement que deux choses: 1. Mettre à jour les détails de la routine, en allant piocher plein d'informations parmi les autres concepts auxquels elle est liée 2. Construire le contexte à afficher, qui ne contient que deux données: la routine 👍 et la liste des compétences auxquelles elle est liée. En fait, **tout** se qui se trouve entre le `get_object_or_404` et le `context = {...}` **doit** se trouver ailleurs que dans cette fonction. C'est impossible à tester ou vérifier de manière automatique si cela se trouve dans une vue et cela viole tous les principes de structuration logique. Copie/colle le directement dans une méthode du modèle que tu pourrais appeler `rebuild` que cela fonctionnerait parfaitement 😉 Une deuxième amélioration possible concerne la lisibilité du bloc: comme tu appelles à chaque fois 28 fois (si, j'ai compté 😛 'fin pas moi tout seul...) `skill_link.skill`, rien que ceci pourrait être renommé en `skill` ou `current_skill` que tu gagnerais 11 caractères (x28). Ensuite, tu as un pattern qui se répète : ``` if <skill>.<property> is not None and <skill>.<property> > <property_aussi>: <property_aussi> = <skill>.<property> ``` Si cela se répète autant, mets le dans une fonction. Un truc tout bête hein, mais un truc quand même: ```python def max_even_if_none(value1, value2): if value1 is not None and value 1 > value2: return value1 return value2 ``` Rien qu'avec ceci, ton code pourrait ressembler à ça: ```python for skill_link in routine.skill_links.all(): skill = skill_link.skill difficulty += skill.difficulty level = max(skill.level, level) rank = max(skill.rank + 1, rank) if not skill.is_competitive: is_competitive = False age_boy_with_help = max_even_if_none(skill.age_boy_with_help, age_boy_with_help) age_boy_without_help = max_even_if_none(skill.age_boy_without_help, age_boy_without_help) age_boy_chained = max_even_if_none(skill.age_boy_chained, age_boy_chained) age_boy_masterised = max_even_if_none(skill.age_boy_masterised, age_boy_masterised) age_girl_with_help = max_even_if_none(skill.age_girl_with_help, age_girl_with_help) age_girl_without_help = max_even_if_none(skill.age_girl_without_help, age_girl_without_help) age_girl_chained = max_even_if_none(skill.age_girl_chained, age_girl_chained) age_girl_masterised = max_even_if_none(skill.age_girl_masterised, age_girl_masterised) ```

Sulley was assigned by Fred

2022-01-08 22:28:20 +01:00

Sulley commented

2022-01-09 11:29:27 +01:00

Un ticket avec 20 milliards d'infos… Hou la la ! t'es un fou toi ! T'as oublié qui je suis :-)

Bon… Premier conseil suivit : j'ai transférer tout dans un méthode de la classe Routine def compute_informations(self):

Je regarde maintenant pour le reste des conseils.

Un ticket avec 20 milliards d'infos… Hou la la ! t'es un fou toi ! T'as oublié qui je suis :-) Bon… Premier conseil suivit : j'ai transférer tout dans un méthode de la classe Routine `def compute_informations(self):` Je regarde maintenant pour le reste des conseils.

Sulley commented

2022-01-09 13:06:47 +01:00

La deuxième partie avance plus lentement :

Traceback (most recent call last):
  File "/Users/redj/Dev/.virtualenvs/ultron/lib/python3.10/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/Users/redj/Dev/.virtualenvs/ultron/lib/python3.10/site-packages/django/core/handlers/base.py", line 181, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/Users/redj/Dev/.virtualenvs/ultron/lib/python3.10/site-packages/django/contrib/auth/decorators.py", line 21, in _wrapped_view
    return view_func(request, *args, **kwargs)
  File "/Users/redj/Sites/Personnal/Ultron/ultron/objective/views.py", line 174, in routine_details
    routine.compute_informations()
  File "/Users/redj/Sites/Personnal/Ultron/ultron/objective/models.py", line 272, in compute_informations
    age_boy_with_help = max_even_if_none(skill.age_boy_with_help, age_boy_with_help)

Exception Type: NameError at /routine/45
Exception Value: name 'max_even_if_none' is not defined

La deuxième partie avance plus lentement : ``` Traceback (most recent call last): File "/Users/redj/Dev/.virtualenvs/ultron/lib/python3.10/site-packages/django/core/handlers/exception.py", line 47, in inner response = get_response(request) File "/Users/redj/Dev/.virtualenvs/ultron/lib/python3.10/site-packages/django/core/handlers/base.py", line 181, in _get_response response = wrapped_callback(request, *callback_args, **callback_kwargs) File "/Users/redj/Dev/.virtualenvs/ultron/lib/python3.10/site-packages/django/contrib/auth/decorators.py", line 21, in _wrapped_view return view_func(request, *args, **kwargs) File "/Users/redj/Sites/Personnal/Ultron/ultron/objective/views.py", line 174, in routine_details routine.compute_informations() File "/Users/redj/Sites/Personnal/Ultron/ultron/objective/models.py", line 272, in compute_informations age_boy_with_help = max_even_if_none(skill.age_boy_with_help, age_boy_with_help) Exception Type: NameError at /routine/45 Exception Value: name 'max_even_if_none' is not defined ```

Sulley referenced this issue from a commit

2022-01-09 15:34:05 +01:00

Refactoring routine_detail function (end). Close #20

Sulley closed this issue

2022-01-09 15:34:05 +01:00

Sign in to join this conversation.