Week 3 assignment: Coder Ticket

[thuanmb]

Hi Charles Lee, Harley Trung,

This's my completed work for week 3 assignment. Please help me review.

Thanks, Thuan //cc: @coderschoolreview

[coderschoolreview]

Thank you for your Ruby submission, thuanmb!

:innocent: Everything looks good with the format of your submission. We'll be providing a detailed review soon!

[harley]

Hi @thuanmb :+1: with the homework. The goal this week was to enhance a basic app with search, complex form, relationships between models and add tests on top of that.

Nice things:

• Nice heroku app name
• Use of textacular for search (it is not necessary in this app but it is useful to try)
• Handling of the complex event creation form
• Very nice tests and good job with simplecov
• Use of rspec shared_context
• You define current_user in ApplicationHelper and include this helper into ApplicationController. So far in case we have only done the opposite: define the methods in ApplicationController and make them available for the view and view helpers. Either way is fine. I see that you use include in a few places in the code. You can read more on Ruby Mixins in general.

Suggestions:

• It'd be more interesting if you had seeded fake events to showcase your site. You can do that by pushing your local data to heroku using the pg:push command: https://devcenter.heroku.com/articles/heroku-postgresql#pg-push-and-pg-pull

• Routes: consider using RESTFUL routes instead of custom routes when appropriate. For example:

  # don't do this
  get 'venues/create'
  get 'sessions/new'
  get 'sessions/create'
  get 'users/new'
  get 'users/create'

  Consider the following instead (and note that for #create, we want post, not get)

  # the `only:` part is not really necessary
  resources :venues, only: [:create]
  resources :sessions, only: [:new, :create]
  resources :users, only: [:new, :create]

• In routes.rb: I see that you use GET routes for requests that make changes to the database. Please use put or delete instead. For example get 'events/:event_id/ticket_type/:ticket_type_id/delete' => 'events#remove_ticket_type' should we

  # you can choose to be explicit about the routes
  delete 'events/:event_id/ticket_type/:ticket_type_id/delete' => 'events#remove_ticket_type'
  
  # or you can go fancy with Rails' nested routes
  # this gives you the following route for free:
  # DELETE /events/:event_id/ticket_types/:id
  # which goes to ticket_types#destroy action instead of events#remove_ticket_type
  resources :events do
  resources :ticket_types
  end

• Your EventsController class contains a little too much logic. Try to keep controllers thin and don't give each too many responsibilities.
• Replace if !@user.presence with if @user.blank? because it means the same. Even better, use if @user.nil? instead because @user cannot be empty. Note the difference between Object#presence and Object#present?. Also you may find it interesting to read about the differences between .nil? .empty? .blank? .present? in Ruby: http://stackoverflow.com/questions/885414/a-concise-explanation-of-nil-v-empty-v-blank-in-ruby-on-rails
• The TicketsController#create action is a little too big. It'd be an improvement if you can move some of the logic into the models (by defining instance or class methods in Event, Ticket, and TicketType models.
• Replace newTicket with new_ticket in tickets_controller.rb file
• In users_controller.rb, instead of .join(', ') in @user.errors.full_messages.join(', '), you can use .to_sentence instead
• I see some tab characters in event.rb and spec/features/events_spec.rb. For Rails projects, set your editor to use two spaces instead of tabs for intentation. I recommend taking a quick read of the ruby style guide and revisit it a few times.

cc @coderschoolreview