huycan / coderschool-blog

A simple Rails blog app for CoderSchool.vn
0 stars 0 forks source link

Pre-work blog app submission #1

Open huycan opened 8 years ago

huycan commented 8 years ago

Hi @coderschoolreview,

Please see the repository for my pre-work submission.

Thank you, and I hope you enjoy this little app!

Huy

coderschoolreview commented 8 years ago

Thank you for your Rails submission, huycan!

:grin: Everything looks good with the format of your submission. We'll be reviewing your submission soon!

nvh0412 commented 8 years ago

Hi @huycan

Thanks for your submission! Good work. :+1:

For a quick review, your submission is good, but if you have free time, you should improve your assignment and try to complete remaining option stories as well.

Suggestions

    @articles = Article.search(params[:search]) if params[:search].present?
    @articles = Article.order(created_at: :desc)

Let us know if you have any questions. /cc @coderschoolreview @harley.

huycan commented 8 years ago

Thanks for your feedback @nvh0412 @coderschoolreview !

The view helpers and ilike are really neat!

I try your refactor, it doesn't work because Article.order query all records rather than the search results. I use collection sort_by instead. This leads me to a question:

I will return to complete the other stories and UI after understanding more about the black box that powers Rails' magic.

nvh0412 commented 8 years ago

Hi @huycan ,

I'm sorry about my code. :). Nice to see you had tried to refactor your code. :100: You can try it again by following:

    @articles = Article.all
    @articles = @articles.search(params[:search]) if params[:search].present?
    @articles = @articles.order(created_at: :desc)

it don't seem like shorter than your recent code, but it doesn't need call order twice in 2 cases. About your question, YES, you can research scope and class method of ActiveRecord. :).

/cc @harley, @coderschoolreview

huycan commented 8 years ago

Hi @nvh0412 ,

You shouldn't apologize, I really appreciate the fact you take time to help me :monkey_face: Great mentor and code review are invaluable.

I agree that DRYing the order clause is good, however, wouldn't Article.all always load all records from DB and thus defeat the purpose of a simple search query? My current code is

@articles = params[:search].present? ? Article.search(params[:search]) : Article.all
@articles = @articles.sort_by { |a| a.created_at } .reverse!

Thanks for pointing me to scope and class method of ActiveRecord. Sweet reading, it answers my question nicely :+1:

nvh0412 commented 8 years ago

Hi @huycan

if you had known about scope of ActiveRecord, you should try to create 2 scopes are search and newest

scope :search, -> (keyword) { where("title ilike ?", "%#{keyword}%") if keyword.persent?  }
scope :newest, -> { order(created_at: :desc) }

#article controller
@articles = Article.search(params[:search]).newest

I don't use class method in here, because scopes will always return a chainable object, but class methods won't, let's assume that keyword is null, if you choose class method for search function, it will be return nil value and you will received undefined method 'newest' for nil class, but you will receive Artcle.all with scope. This is a difference between scope and class method of active record.

I'm very excited about what you've done. :+1: See you tomorrow.

/cc @harley, @coderschoolreview