Feedback général sur ton code

MaxLap commented 4 years ago

Salut, je suis passer rapido sur ton code. Voici donc du feedback.

Namespacing controllers et vues

Pour les controlleurs et les vues, tu veux probablement namespacer ça un peu. Il y a 2 sections: administration et customer. Ce qui risque de se passer, c'est que les deux vont vouloir avoir des actions en lien avec le même objet (ex: room). Mettre tout ça dans le même controlleur va être dérangeant. Donc ce qui est mieux, c'est de faire une hierarchie où toutes les vues liés à l'administration sont dans des dossiers sous le dossier "administration", puis même chose pour le côté "customer". Point de vue controlleur, même chose, tous les controllers (sauf application et autres qui sont partagés) sont dans les dossiers administration ou customer. Point de vue routes, il faut mettre le tout comme ça

namespace :administration do
  # toutes les routes liés à administration
  resources :room_types, only: [:index, :create]
end

RoomPricingsController

def show
    @room_pricing = RoomPricing.find(params[:id])
    @room_type = RoomType.find(@room_pricing.room_type_id)
    @room_view = RoomView.find(@room_pricing.room_view_id)
    @rooms = Room.where(room_type_id: @room_pricing.room_type_id, room_view_id: @room_pricing.room_view_id).order("number ASC")
  end

Dans le code ci-haut, plutôt que de faire RoomType.find(...), tu peux faire @room_pricing.room_type directement. C'est l'un des effects d'avoir déclaré le belongs_to :room_type. Même chose pour room_view.

Comme c'est quelque chose qu'on pourrait imaginer arrive souvent, plutôt que de mettre une longue requête pour trouver les rooms qui matchent le pricing, tu pourrais vouloir faire une méthode sur @room_pricing qui fait ça pour toi. Tu pourrais alors faire

@rooms = @room_pricing.matching_rooms.order("number ASC")

Ta méthode dans RoomPricing serait

def matching_rooms
  Room.where(room_type_id: room_type_id, room_view_id: room_view_id)
end

Administration side menu

Plutôt que de faire des = render "administration/side_menu" dans chaque vue, tu devrais faire un layout "administration". Je te recommende d'utiliser la gem https://github.com/rwz/nestive pour faire ça (la méthode extends)

CebDev commented 4 years ago

Merci pour ton retour, je vais regarder ça point par point et apporter les corrections nécessaires.

Pour le reste dans l'ensemble, cela te parait correct ? par exemple dans la gestion des formulaires et la syntaxe utilisée ? Je ne veux pas prendre de mauvaises habitudes et corriger le plus rapidement possible. Au niveau des controlleurs je sais que je dois faire du refactoring.

MaxLap commented 4 years ago

Pour le reste ça semble bien. Les formulaires me semblent bien. Je n'ai pas d'expérience avec bootstrap, mais j'imagine que dans une vrai application, on essaierait de faire des outils pour éviter de toujours mettre les divs de bootstrap manuellement et pour uniformiser les formulaires. Mais pour cette app, ya pas de problème.

J'ai oublié de mentionné, mais normalement, les flash messages sont gérés dans le layouts.

CebDev / NaganoConfortInn