Assignment 1 Review

[nongdenchet]

You have done most part of the homework, really appreciate your work

Goals of this homework:

• Learn how to define root path and custom paths. Use _path methods in controller and view files
• Use ActiveRecord querying with .where
• Create multiple ActiveRecord models with different types of columns
• Sort items by using ActiveRecord's .order method (reference here)
• Use ActionController's Flash to display success or failure notices (reference here)

Great stuffs:

• Good use of raty.js library
• Good use of has_many and belongs_to relationship, you look confident to use these methods
• Beautiful User Interface
• Really good use of validation on models
• You look confident using order and where, really awesome stuffs
• You have already used session fluently, great job man
• Very nice way to validate email

• Very nice way of rendering, you know a lot of stuffs man

  <%= render partial: "comments/form", locals: {comment: @comment, item_id: @item.id} %>

Suggestions:

• Its better to use new_orders_path then /orders/new
• When things become more complicated, do you feel the order_controller.rb is a little bit messy? It is doing a lot of stuffs. Good practices are to use PORO you can check it out here. Or you can try to put some code like calculate_discount_price to the model layer

• This code:

  <div class="btn-group" role="group" aria-label="section">
  <%= link_to "Breakfast", menus_path(section: "Breakfast"), class: "btn btn-secondary #{'active' if @section == 'Breakfast'}" %>
  <%= link_to "Lunch", menus_path(section: "Lunch"), class: "btn btn-secondary #{'active' if @section == 'Lunch'}" %>
  <%= link_to "Dinner", menus_path(section: "Dinner"), class: "btn btn-secondary #{'active' if @section == 'Dinner'}" %>
  <%= link_to "Drinks", menus_path(section: "Drinks"), class: "btn btn-secondary #{'active' if @section == 'Drinks'}" %>
  </div>

  can be refactor something like:

  # In controller do this
  @sections = ["Breakfast", "Lunch", "Dinner", "Drinks"]
  
  # And in views you can do:
  <div class="btn-group" role="group" aria-label="section">
  <% @sections.each do |section| %>
    <%= link_to section, menus_path(section: section), class: "btn btn-secondary #{'active' if @section == section}" %>
  <% end %>
  </div>

  or even better way:

  # Create a partial view name it: sections/_section.html.erb
  <%= link_to section, menus_path(section: section), class: "btn btn-secondary #{'active' if selected_section == section}" %>
  
  # In controller do this
  @sections = ["Breakfast", "Lunch", "Dinner", "Drinks"]
  
  # And in views you can do:
  <div class="btn-group" role="group" aria-label="section">
  <%= render partial: 'sections/section', collection: @sections, as: :section, locals: {selected_section: @section}
  </div>

  actually there are a better way then this (which I think the best way for me), but we can talk about it later, its more advance and architecture matter

• This code:

  @section = "Breakfast"
  if params[:section] == "Breakfast"
  @section = "Breakfast"
  elsif params[:section] == "Lunch"
  @section = "Lunch"
  elsif params[:section] == "Dinner"
  @section = "Dinner"
  elsif params[:section] == "Drinks"
  @section = "Drinks"
  end

  can be refactored into something shorter

  sections = ["Breakfast", "Lunch", "Dinner", "Drinks"]
  if sections.include?(params[:section])
  @section = params[:section]
  else
  @section = "Breakfast"
  end

  or something even shorter

  def index
  @section = valid_section? ? params[:section] : "Breakfast"
  ...
  end
  
  def valid_section?
      sections = ["Breakfast", "Lunch", "Dinner", "Drinks"]
  sections.include?(params[:section])
  end

  these approaches produce cleaner and more readable code, because someone may have to maintain your code

Overall, you really push effort to do the homework. A lot of nice stuffs have been done. I love your submission most. Please keep it up

[leo-le-07]

@nongdenchet thanks so much for your feedbacks and advices

[nongdenchet]

Nice, can I have your facebook @leo-le-07