Newbie code review

Hi All,

I am learning rails at the moment and have gone through one of the tutorials on the rails website for creating a simple blog system.

I have added some new features to it and it is working great.

However, I would like to show someone my code and see if it is the right or most efficient way of achieve this.

This system is based on the blog system from the Getting Started with Rails guide which can be found on http://guides.rubyonrails.org/getting_started.html

I simply added a rank up/rank down function to the blog system:

First, in my routes.rb I added:

resources :articles do

resources :comments

member do

get ‘rankup’

get ‘rankdown’

end

end

``

Then, in my controller I added two new actions:

def rankup

@this_article = Article.find(params[:id])

@new_rank = @this_article.rank.to_i-1

@prev_article = Article.find_by(rank: @new_rank)

@prev_article.rank = @this_article.rank

@this_article.rank = @new_rank

@this_article.save

@prev_article.save

redirect_to articles_path

end

def rankdown

@this_article = Article.find(params[:id]) @new_rank = @this_article.rank.to_i+1

@next_article = Article.find_by(rank: @new_rank)

@next_article.rank = @this_article.rank

@this_article.rank = @new_rank

@this_article.save

@next_article.save

redirect_to articles_path

end

``

I also updated the destroy action to include a re ranking function:

def destroy

@article = Article.find(params[:id])

@start_rank = @article.rank

@next_articles = Article.where([“rank > ?”, @start_rank]).order(‘rank ASC’)

@next_articles.each do |article| 

article.rank = @start_rank

article.save

@start_rank = @start_rank + 1 end

@article.destroy

redirect_to articles_path

end

``

And in the view I simply added the links to the list:

<% @articles.each.with_index do |article, index| %>

<%= article.title %> <%= article.text %> <%= article.rank %> <%= link_to 'View', article_path(article) %> <%= link_to 'Edit', edit_article_path(article) %> <%= link_to 'Delete', article_path(article), method: :delete, data: {confirm: 'Are you sure?'} %>

<% if index != 0 %>

<%= link_to ‘Up’, rankup_article_path(article) %>

<% end %>

<% if index != @articles.count-1 %>

<%= link_to ‘Down’, rankdown_article_path(article) %>

<% end %>

<% end %>

``

As mentioned, I am new to RoR so I don’t know if I’m doing this correctly according the Rails convention but the code is working great so I’m happy about that.

If someone can review my code please and tell me what I can improve on, that would be great.

I’m also thinking there might be an existing gem or something that I can install that will do the ranking for me automatically.

Anyway, look forward to your feedbacks.

Thanks in advance.

Don't use GET for actions which change the database, always use POST for such actions. Otherwise imagine the chaos that could be induced when Google scrapes your site following all the links and accidentally tries to rankup/down.

In fact Elizabeth (or should that be Ms McGurty?) is correct to suggest that the actions might be better rolled into the update action. You can add parameters to the action to indicate what the action is.

Colin

What is the point of starting from scratch when one has a working application already, unless there is something major wrong with the existing application?

Colin

Hi All,

[snip]

As mentioned, I am new to RoR so I don’t know if I’m doing this correctly according the Rails convention but the code is working great so I’m happy about that.

If someone can review my code please and tell me what I can improve on, that would be great.

In addition to what Colin has pointed out, I’d also move the code for updating an object’s rank into the model - it will make your controller much simpler and also make it easier to test. You could probably also take the opportunity to reduce some of the duplication in the rank up/down method.

On a substance rather than style issue, you don’t handle some of the edge cases, such as what if there is no next article?

The only other real issue is that if two people try to update 2 objects at the same time there is a chance that the 2 updates will clash and you could end up with duplicates or gaps - in a toy app like this, largely a theoretical concern, but worth at least understanding that it could happen.

I’m also thinking there might be an existing gem or something that I can install that will do the ranking for me automatically.

acts_as_list handles most if not all of this. You’ll probably learn more rolling your own though!

Fred

Hi Fred,

I took your advice and tried to move the ranking logic over to the model.

I’ve also consolidated the 2 actions.

Here’s what I got:

In the controller:

def rank

@this_article = Article.find(params[:id])

if (params[:rank] == ‘up’)

@next_article = Article.find_by(rank: @this_article.rank.to_i-1)

@this_article.rankup

@next_article.rankdown

else

@next_article = Article.find_by(rank: @this_article.rank.to_i+1)

@this_article.rankdown @next_article.rankup

end

@this_article.save

@next_article.save

redirect_to articles_path

end

``

In the view, I am passing in the query string rank to indicate whether we are ranking up or down.

And in my model I added two new methods:

def rankup

self.rank = self.rank - 1

end

def rankdown

self.rank = self.rank + 1

end

``

I’ve added the rank up / rank down as a method in the Article object. However, the key to my ranking logic is to first find the next record as I need to swap the ranking with the current record. So the code to finding the next record (either previous or next record depending on whether we are ranking up or down). This code can only reside in the controller as far as I can tell.

Please let me know if there is more code I can move to the model.

Thank you.

Why do you think you cannot move more to the model? Which bit of code in particular cannot reside there?

Colin

Hi All,

I have moved everything over to the model as suggested. Check it out :stuck_out_tongue:

In the model, I created a Class method as well as the previous instance methods:

class Article < ActiveRecord::Base

def self.rank(id, rank)

@this_article = find(id)

if (rank == “up”)

@next_article = find_by(rank: @this_article.rank.to_i-1)

@this_article.rankup

@next_article.rankdown

else

@next_article = find_by(rank: @this_article.rank.to_i+1)

@this_article.rankdown

@next_article.rankup

end

end

def rankup

self.rank = self.rank - 1

self.save

end

def rankdown

self.rank = self.rank + 1

self.save

end

end

``

And now in my controller, I only have one action:

def rank

Article.rank(params[:id], params[:rank])

redirect_to articles_path

end

``

It is working beautifully so I’m happy about that. However, can anyone please review the code and let me know if it is correct in terms of OOP?

And if it is best practice for Rails.

Rails is awesome.

Cheers

Rather than having a class method rank() I think it would be better to have a member method then in the controller do something like this_article = Article.find(params[:id]) this_article.rank( params[:rank] )

Also don't forget to allow for the fact that there might not be an article with the given id, so perhaps this_article.rank (params[:rank]) if this_article

Colin

Maybe take out the save from rankup and rankdown and save in the controller?

Yes but I also call those method from the Class method rank and it needs to save.

This is always my dilemma, just exactly what or how much code should go into controller and model?

Like in my example, before when I had everything in the controller, people suggested that I put that code in the model.

So I did. But now, people are suggesting I take some code back out to the controller.

So exactly which part should go into the controller and which part belongs to the model?

Having said that, I do agree the self.save should be in the controller but this will break my self.rank Class method. Arrrgh!

Any suggestion how I can break down the self.rank Class method so I can put the self.save into the controller?

Thanks

Hi All,

First of all, let me quickly clarify the meaning of ranking here.

All I’m doing is move an article up or down in the display on the index file.

Quick example. Below is a sample of the Articles table:

ID title ranking

5 Rails is cool 1

6 Programming is fun 2

7 How to install Rails 3

8 Macbook Pro features 4

If a user click on article ID 7 to move it up, this is what the system should do:

Change the ranking for article ID 7 to 2

Change the ranking for article ID 6 to 3

Basically, swap them around.

So as you can see, when a user click on an article, the system needs to modify 2 records in the db.

Now, if I want to move the self.save to the controller, the best way I can think of the implement this is as follows:

IMPORTANT: I have renamed the column rank to ranking below

In my model, I have 3 methods:

def self.find_by_rank(id, rank)

@this_article = find(id)

if (rank == “up”)

@next_article = find_by(ranking: @this_article.ranking.to_i-1)

else

@next_article = find_by(ranking: @this_article.ranking.to_i+1)

end

end

def rankup

self.ranking = self.ranking - 1

end

def rankdown

self.ranking = self.ranking + 1

end

``

And in my controller, I have this:

def rank

@this_article = Article.find(params[:id])

@affected_article = Article.find_by_rank(params[:id], params[:rank])

if (params[:rank] == ‘up’)

@this_article.rankup

@affected_article.rankdown

else

@this_article.rankdown

@affected_article.rankup

end

@this_article.save

@affected_article.save

redirect_to articles_path()

end

``

This is the best way I can think of the move the self.save to the controller while keeping the majority of the logic in the model.

Please let me know if there is a better way.

Thank you.

Don't move it to the controller. If it affects multiple records in the db then put it in the model. In addition wrap the complete operation in a transaction so that you are guaranteed to do both or none, otherwise you will end up with the db in an illegal state.

However, if you are effectively maintaining an ordered list then I suggest looking at the gem acts_as_list which will take much of the hard work out of this for you.

Colin

Yeah, I will check that gem out. I basically wanted to do this to learn the ways of the Rails :stuck_out_tongue:

Yeah, I will check that gem out. I basically wanted to do this to learn the ways of the Rails :stuck_out_tongue:

In that case I would have two member methods rankup and rankdown that do everything, including saving, all wrapped in a transaction.

So controller code is just something like   @this_article = Article.find(params[:id])   if params[:rank] == 'up'     @this_article.rankup   elsif params[:rank] == 'down'     @this_article.rankdown   else     # cope with this error appropriately   end

Then you have moved everything that needs to know how ranking works into the model, while the controller deals with parameters.

Within the model the up/down might be performed using a class method swap_rank( article_1, article_2 ) with rankup and rankdown finding the articles affected.

Colin

To be completely honest, it’s almost worth doing it the wrong way - until you’ve done it wrong (and suffered the eventual consequences) it probably does sounds like a conflicting bunch of edicts handed down from on high. Unfortunately it sometimes takes a while for the wrongness to become apparent, and it’s not always easy to spot what is hard because you’re just new to it and it’s normal versus what you’ve made hard for yourself from a poor design decision,

I think I’m basically saying that this will come with experience and don’t sweat it too much to begin with. Breaking up the rank method so that the save is separate from the actual modification sounds like a bad idea though (as Colin has said)

Fred

OK, so I have modified the code following Colin’s suggestion.

Check it out:

In the controller, my action is as follows:

def rank

@this_article = Article.find(params[:id])

if (params[:rank] == ‘up’)

@this_article.rankup(params[:rank])

else

@this_article.rankdown(params[:rank])

end

redirect_to articles_path()

end

``

In my model, I have rankup and rankdown instance methods then I have a private method:

def rankup(rank)

swap_rank(self.id, self.ranking, rank)

self.ranking = self.ranking - 1

self.save

end

def rankdown(rank)

swap_rank(self.id, self.ranking, rank)

self.ranking = self.ranking + 1

self.save

end

private

def swap_rank(id, ranking, rank)

if (rank == ‘up’)

@affected_article = Article.find_by(ranking: ranking.to_i-1)

@affected_article.ranking = @affected_article.ranking + 1

else

@affected_article = Article.find_by(ranking: ranking.to_i+1)

@affected_article.ranking = @affected_article.ranking - 1

end

@affected_article.save

end

``

I like this way. It is clean and simple.

All the code that make changes to the db resides in the model and the controller simply determines which model method to call.

Thanks, Colin.

A few points. What happens if params[:rank] is not specified or is not valid? What would the effect be if self.save in rankup failed (a validation failure for example), or the affected_article.save failed? What would the affect be if your app or the server crashed between the two saves? As I previously suggested wrap the whole thing in a transaction in order to recover from this sort of problem. In order to do that change swap_rank to do the complete operation including both saves, possibly by passing it the two articles to be swapped.

Colin

Oh yeah, like this?

In controller:

def rank

@this_article = Article.find(params[:id])

if (params[:rank] == ‘up’)

@this_article.rankup

else

@this_article.rankdown

end

redirect_to articles_path()

end

``

In the model:

def rankup()

@affected_article = Article.find_by(ranking: ranking.to_i-1)

self.ranking = self.ranking - 1

@affected_article.ranking = @affected_article.ranking + 1

Article.transaction do

self.save

@affected_article.save

end

end

def rankdown()

@affected_article = Article.find_by(ranking: ranking.to_i+1)

self.ranking = self.ranking + 1

@affected_article.ranking = @affected_article.ranking - 1

Article.transaction do

self.save

@affected_article.save

end

end

``

Too much repeated code How about def rankup   @affected_article = Article.find_by(ranking: ranking.to_i-1)   adjust_ranks this, @affected_article end

etc.

Colin

Had a little fun with acts_like_list… Build in Rails 3, should work in Rails 4 though??? Add to your gem file: gem ‘acts_as_list’ Run: bundle install Add the field position as an integer to your Articles table: From rails console: RUN: rails g migration AddPositionToArticle position:integer RUN: rake db:migrate You could go to your database to verify