fix(request): ajout du handler_id de user dans une request #94
Conversation
|
Tao-Galasse
left a comment
Tao-Galasse
left a comment
There was a problem hiding this comment.
Les détails sur les seeds reste avant tout des conseils d'organisation de code et de relecture ; beaucoup de méthodes dans un même fichier s'appelant les unes les autres et avec peu de consistance diminuent la maintenabilité, et il vaut mieux toujours avoir en tête la question "comprendrais-je toujours ce que fait mon algo en le relisant dans 6 mois ?".
Mais ça ne reste que du pinaillage sur un fichier qui ne sert de toute façon qu'à alimenter la bdd avec des données pseudo-aléatoires, donc ça n'est pas bien important.
En revanche, pour ce qui touche au code fonctionnel de l'application, ça aurait été bien de le tester, c'est beaucoup plus important 👍
| requests = [] | ||
| 5.times do |i| | ||
| plant = Plant.order('RANDOM()').first | ||
| plant_stage = plant.plant_stages.order('RANDOM()').first | ||
| requests << Request.create!( | ||
| requester_first_name: Faker::Name.first_name, | ||
| requester_last_name: Faker::Name.last_name, | ||
| requester_email: Faker::Internet.email, | ||
| laboratory: Faker::Science.scientist, | ||
| handler: User.first, | ||
| plant_stage: plant_stage, | ||
| name: "Requête #{i + 1}", | ||
| plant_name: plant.name, | ||
| plant_stage_name: plant_stage.name, | ||
| comment: Faker::Lorem.sentence, | ||
| due_date: Date.current + rand(1..6).months, | ||
| quantity: rand(10..100), | ||
| temperature: %w[Chaud Froid Extérieur].sample, | ||
| photoperiod: rand(6..18), | ||
| status: %i[pending refused completed].sample | ||
| ) | ||
| end |
There was a problem hiding this comment.
on peut utiliser sample plutôt que de faire un réordonnement aléatoire, et utiliser le map pour mettre directement le résultat dans l'objet requests sans avoir besoin d'utiliser le << à chaque itération :) (c'est purement du pinaillage syntaxique hein, on est bien d'accord)
| requests = [] | |
| 5.times do |i| | |
| plant = Plant.order('RANDOM()').first | |
| plant_stage = plant.plant_stages.order('RANDOM()').first | |
| requests << Request.create!( | |
| requester_first_name: Faker::Name.first_name, | |
| requester_last_name: Faker::Name.last_name, | |
| requester_email: Faker::Internet.email, | |
| laboratory: Faker::Science.scientist, | |
| handler: User.first, | |
| plant_stage: plant_stage, | |
| name: "Requête #{i + 1}", | |
| plant_name: plant.name, | |
| plant_stage_name: plant_stage.name, | |
| comment: Faker::Lorem.sentence, | |
| due_date: Date.current + rand(1..6).months, | |
| quantity: rand(10..100), | |
| temperature: %w[Chaud Froid Extérieur].sample, | |
| photoperiod: rand(6..18), | |
| status: %i[pending refused completed].sample | |
| ) | |
| end | |
| requests = 5.times.map do |i| | |
| plant = Plant.all.sample | |
| plant_stage = plant.plant_stages.sample | |
| Request.create!( | |
| requester_first_name: Faker::Name.first_name, | |
| requester_last_name: Faker::Name.last_name, | |
| requester_email: Faker::Internet.email, | |
| laboratory: Faker::Science.scientist, | |
| handler: User.first, | |
| plant_stage:, | |
| name: "Requête #{i + 1}", | |
| plant_name: plant.name, | |
| plant_stage_name: plant_stage.name, | |
| comment: Faker::Lorem.sentence, | |
| due_date: Date.current + rand(1..6).months, | |
| quantity: rand(10..100), | |
| temperature: %w[Chaud Froid Extérieur].sample, | |
| photoperiod: rand(6..18), | |
| status: %i[pending refused completed].sample | |
| ) | |
| end |
| def select_random_bench_and_pot | ||
| [Bench.order('RANDOM()').first, Pot.order('RANDOM()').first] | ||
| end |
There was a problem hiding this comment.
idem ici, on peut utiliser Bench.all.sample et Pot.all.sample. Et quitte à faire ça, je pense qu'on pourrait complétement se débarrasser de la méthode select_random_bench_and_pot, pas très utile ici car utilisée une seule fois, et directement utiliser nos .sample là où on en a besoin, dans la méthode distribute_request (ça permet aussi de directement récupérer notre bench et notre pot random, sans avoir à définir un tableau dans une méthode, pour déstructurer ce tableau dans la méthode appelante)
| def create_distribution_record(request:, bench:, pot:, details:) | ||
| create_request_distribution( | ||
| request: request, | ||
| bench: bench, | ||
| pot: pot, | ||
| plant_stage: request.plant_stage, | ||
| quantity: details[:quantity], | ||
| position: details[:position], | ||
| dimensions: details[:dimensions] | ||
| ) | ||
| end |
There was a problem hiding this comment.
pourquoi avoir défini une autre méthode qui va de toute façon réutiliser les mêmes clés ?
| def create_distribution_record(request:, bench:, pot:, details:) | |
| create_request_distribution( | |
| request: request, | |
| bench: bench, | |
| pot: pot, | |
| plant_stage: request.plant_stage, | |
| quantity: details[:quantity], | |
| position: details[:position], | |
| dimensions: details[:dimensions] | |
| ) | |
| end | |
| def create_distribution_record(request:, bench:, pot:, details:) | |
| RequestDistribution.create!( | |
| request:, | |
| bench:, | |
| pot:, | |
| plant_stage: request.plant_stage, | |
| pot_quantity: details[:quantity], | |
| positions_on_bench: details[:position], | |
| dimensions: details[:dimensions] | |
| ) | |
| end |
et avec ça, plus besoin de la méthode create_request_distribution
| def overlap?(position, dimensions, occupied) | ||
| ox, oy, ow, oh = occupied | ||
| px, py = position | ||
| pw, ph = dimensions | ||
|
|
||
| !(px + pw <= ox || px >= ox + ow || py + ph <= oy || py >= oy + oh) | ||
| end |
There was a problem hiding this comment.
ça me semble étonnant d'avoir eu à redéfinir ça ici (alors que ça ne semble pas vraiment être une logique liée au fait de seeder des données), alors qu'on a déjà un algo quasi similaire sur un modèle de mémoire 🤔 on aurait sûrement pu l'utiliser, et garder une conception beaucoup plus orientée objet dans ce fichier de seed plutôt que de tout faire de façon très algorithmique et moins lisible :)
| { | ||
| dimensions: dimensions, | ||
| position: position, | ||
| quantity: distributed_quantity, | ||
| total_quantity: request.quantity | ||
| } |
There was a problem hiding this comment.
Pourquoi avoir défini un hash ici ? Ça fait qu'entre nos méthodes, on utilise tantôt des variables "claires" (request, bench, pot, etc) et parfois des hash avec des noms génériques et changeants où il est difficile de savoir ce qu'on retrouve dedans (details, distribution_details, etc). Autant n'utiliser que des variables "simples", ça vous aidera aussi à la relecture et à la maintenabilité :)
| def find_position_for_request(bench, dimensions) | ||
| occupied_positions = [] | ||
| find_available_position(bench, dimensions, occupied_positions) | ||
| end |
There was a problem hiding this comment.
cette méthode semble avoir assez peu d'intérêt ; on aurait sûrement pu la fusionner avec find_available_position, puisque celle-ci se contente juste d'appeler la suivante en lui passant un tableau vide
There was a problem hiding this comment.
c'est bien d'avoir ajouté ça dans le controller, mais ça aurait été mieux de mettre à jour les tests pour vérifier que ces valeurs sont bien sauvegardées dans la BDD lors de l'appel à ces endpoints 👍
No description provided.