[Ruby][Assignment 1] - Review my app

[minhchau273]

Hi @harley, Could you please review my homework? Sorry I didn't complete all the required feature :disappointed: Thank you!

[harley]

Great stuff!!

• first and foremost: test coverage. wow! i won't give feedback on your tests this time. we only cover tests in week 3 :) when you add the spec folder and not use the test folder, consider removing it
• clean code. keep following a good style guide such as the github ruby style guide -- the rails style guide can also be useful, but note that different organizations and companies may have their own variations.
• using validation
• using .ruby-version and .ruby-gemset with RVM!
• nice choise of nav-tabs for section

Suggestions:

• the sort dropdown is great. perhaps put it on the right hand side (while the nav tabs are still on the left hand side. e.g. separate them using .col-sm-8 and .col-sm-4. that way, on mobile screens, they each take the full width.
• add more validation on important fields, such as in MenuItem and Section
• try to use only 2 soft spaces indentation. e.g. in MenuItem model it looks weird:

  def image_url_or_default
    if image_url.present?
      image_url
    else
      "#{IMAGE_BASE_URL}#{CGI.escape name}"
    end
  end

Additionally, I know I wrote this code in the walkthrough so that it's readable to new Rubists. As you are more comfortable Ruby, you can write it more idiomatically as

def image_url_or_default
  image_url.presence || "#{IMAGE_BASE_URL}#{CGI.escape name}"
end

• Nice use of the number_with_precision helper and it is cool that you know how to include the helper methods to a model. You can also check out number_to_currency method (although it would take a few arguments to display nicely in VND). In general, avoid having to use helper methods in a model, but your use case is fine.
• In MenuController, it's awkward to deal with section in various places. You can move the sort logic into the model. The menu#index action can be simplified to (note how I remove many lines that deal with section)

# file: menu_controller.rb
def index
  @sections = Section.all
  @section = Section.find_by_id(params[:session]) || Section.first

  @sort = params[:sort] || "alphabetical"
  @menu_items = MenuItem.filter(@section.menu_items, @sort)
end

Note you'd have to define def self.filter yourself in MenuItem model.

You can improve it further by:

• using ActiveRecord order("price") or order("price DESC") to sort instead of using sort_by. See the official guide.
• chain the where and order methods together.
• check this out for a more generic way to display flash messages: https://makandracards.com/brainchild/33159-bootstrap-3-and-rails-4-flash-messages -- this refactoring covers a few core concepts. Let me know if you have questions on them.
• use params[:session_id] instead of params[:session] -- use the _id suffix for param keys that are for model ids.

Thanks for giving this homework a try. Look forward to your feedback on ways to make it better as well.

[chug2k]

+1. What we really need is a set of guides and/or guides (documents) to help explain certain key concepts useful in this homework.

Amazing job to both of you.