Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ajout d'exercices pour les GRASP #111

Merged
merged 3 commits into from
Feb 20, 2023
Merged

Ajout d'exercices pour les GRASP #111

merged 3 commits into from
Feb 20, 2023

Conversation

Olon11
Copy link
Collaborator

@Olon11 Olon11 commented Feb 19, 2023

@Olon11 Olon11 self-assigned this Feb 19, 2023
@Olon11 Olon11 requested a review from fuhrmanator February 19, 2023 16:58
@Olon11 Olon11 marked this pull request as ready for review February 19, 2023 16:58
fuhrmanator
fuhrmanator reviewed Feb 19, 2023
View reviewed changes
Copy link
Owner

@fuhrmanator fuhrmanator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ça me semble bien comme ajout. Je dois vérifier la mise en page PDF (plus tard).

@fuhrmanator
Copy link
Owner

J'ai tenté faire une solution cohérente avec la solution au problème de polymorphisme, dans la fabrique on y ajoute un type ayant les 3 possibilités dans addBird(). Ça élimine le cas d'erreur. Mais, ça risque d'être compliqué à comprendre pour les débutants en TypeScript. On pourrait faire un lien vers le Playground.

@fuhrmanator
Mise en page (PDF),
63d42e5 
l'impact des nouvelles classes Bird est moins (mais pas zéro) sur la fabrique (si un nouveau a un autre attribut, `addBird` va changer)
fuhrmanator
fuhrmanator approved these changes Feb 19, 2023
View reviewed changes
Copy link
Owner

@fuhrmanator fuhrmanator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai fait un commit avec qqs changements. Tu confirmes si tu veux qu'on fusionne.

@Olon11
Copy link
Collaborator Author

Olon11 commented Feb 19, 2023

Je suis d'accord avec les changements.

J'ai tenté faire une solution cohérente avec la solution au problème de polymorphisme, dans la fabrique on y ajoute un type ayant les 3 possibilités dans addBird(). Ça élimine le cas d'erreur. Mais, ça risque d'être compliqué à comprendre pour les débutants en TypeScript. On pourrait faire un lien vers le Playground

Je ne suis pas certain de comprendre. Ces changements n'apparaissent pas dans ton commit. À moins que ce soit juste une réflexion...

@Olon11
Copy link
Collaborator Author

Olon11 commented Feb 20, 2023

#59

@fuhrmanator
Copy link
Owner

Ces changements n'apparaissent pas dans ton commit. À moins que ce soit juste une réflexion...

C'est dans le lien du Playground (vois-tu le code)? Je disais que ce lien peut être éventuellement une solution alternative "avancée" en TypeScript, mais l'expliquer est p-e trop long.

@Olon11
Copy link
Collaborator Author

Olon11 commented Feb 20, 2023

Oui, j'ai vu le code dans le lien. Le commentaire était ambigu.

Effectivement, ça pourrait être confus pour les lecteurs qui ne sont pas familiers avec TypeScript. Je crois que c'est mieux de conserver le code plus simple.

@fuhrmanator fuhrmanator merged commit 6898a1e into main Feb 20, 2023
@fuhrmanator
Copy link
Owner

Merci @Olon11 !

@fuhrmanator fuhrmanator deleted the olon11/grasp2 branch February 20, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuhrmanator fuhrmanator fuhrmanator approved these changes

Assignees

@Olon11 Olon11

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Olon11 @fuhrmanator