Newbie code review

[Federicko]

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| %>

    <tr>

      <td><%= article.title %></td>

      <td><%= article.text %></td>

      <td><%= article.rank %></td>

      <td><%= link_to 'View', article_path(article) %></td>

      <td><%= link_to 'Edit', edit_article_path(article) %></td>

      <td><%= link_to 'Delete', article_path(article), method: :delete, data: {confirm: 'Are you sure?'} %></td>

      <td>

        <% if index != 0 %>

          <%= link_to 'Up', rankup_article_path(article) %>

        <% end %>

      </td>

      <td>

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

          <%= link_to 'Down', rankdown_article_path(article) %>

        <% end %>

      </td>

    </tr>

  <% 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.

[Colin Law]

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

[Colin Law]

On 28 June 2015 at 22:15, Elizabeth McGurty <emcg...@gmail.com> wrote:
> Jeez.... For a first time effort, you have done an excellent job... Seems
> that you have great instincts..
>
> I don't see your form here, but defs rankup an rankdown should be
> consolidated into a controller action 'update'. What's the name of your
> view? Seems here it should be called list.
>
> If I were you, I would start from scratch... Seems to me that you didn't use
> Rails generators in your process.

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

[Frederick Cheung]

On Sunday, June 28, 2015, Federicko <fedo...@gmail.com> wrote:

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

 

Anyway, look forward to your feedbacks.

Thanks in advance.

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Talk" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rubyonrails-ta...@googlegroups.com.
To post to this group, send email to rubyonra...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rubyonrails-talk/73894ed9-0292-469d-9741-534e50917f04%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

 

[Federicko]

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. 

[Colin Law]

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

Colin

[Federicko]

Hi All,

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

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

[Colin Law]

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

[Ivan Lasorsa]

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

--
Posted via http://www.ruby-forum.com/.

[Federicko]

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

[Federicko]

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.

[Colin Law]

On 1 July 2015 at 05:06, Federicko <fedo...@gmail.com> wrote:
> 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:

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

[Federicko]

Yeah, I will check that gem out.

I basically wanted to do this to learn the ways of the Rails :p

[Colin Law]

On 1 July 2015 at 09:33, Federicko <fedo...@gmail.com> wrote:
> Yeah, I will check that gem out.
> I basically wanted to do this to learn the ways of the Rails :p

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

>
> --
> You received this message because you are subscribed to the Google Groups
> "Ruby on Rails: Talk" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to rubyonrails-ta...@googlegroups.com.
> To post to this group, send email to rubyonra...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/rubyonrails-talk/acd8dac8-a5a2-4b8f-abfc-3e72ad12b255%40googlegroups.com.

[Frederick Cheung]

On Tuesday, June 30, 2015 at 4:46:40 PM UTC+1, Federicko wrote:

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!

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

[Federicko]

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.

[Colin Law]

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

[Federicko]

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

[Colin Law]

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

[Elizabeth McGurty]

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
mysql> describe articles;
+------------+--------------+------+-----+---------+----------------+
| Field      | Type         | Null | Key | Default | Extra          |
+------------+--------------+------+-----+---------+----------------+
| id         | int(11)      | NO   | PRI | NULL    | auto_increment |
| article    | varchar(255) | YES  |     | NULL    |                |
| rank       | int(11)      | YES  |     | NULL    |                |
| position   | int(11)      | YES  |     | NULL    |                |
+------------+--------------+------+-----+---------+----------------+
Go to your Article model:
class Article < ActiveRecord::Base
  attr_accessible :article, :rank, :position
  acts_as_list
  before_save :position_to_rank
  ## Just populating rank .... eventually you can delete it, at view level you can use the work Rank if you are more comfortable with that
  def position_to_rank
    self.rank = self.position
  end
end
Your Articles controller:
class ArticlesController < ApplicationController
 
 
  def index
      @articles = Article.order("position")
    
  end

  def rank
     
      @art = Article.find(params[:id])
      process  = params[:process]
      if process == "Move Up"
        @art.move_higher
      elsif process == "Move Down" 
       @art.move_lower
      elsif process == "Move Bottom" 
       @art.move_to_bottom
     elsif process == "Move Top" 
       @art.move_to_top
     elsif process == "Insert At" 
      @art.insert_at(params[:id].to_i - 1)
     end
      redirect_to :action => :index
  end
 
  def show
      @article = Article.find(params[:id]) unless params[:id].blank?
  end

  def new
      @article = Article.new
  end

  def create
    ## Didn't test this, obviously no error management
    @article = Article.new(params[:article]) unless params[:article].blank?
    if @article.save
      redirect_to @article, :notice => "Successfully created article."
    else
      render :action => 'new'
    end
  end

  def edit
    ## Didn't test this, obviously no error management
   @article = Article.find(params[:id]) unless params[:id].blank?
  end

  def update
    ## Didn't test this, obviously no error management
    @article = Article.find(params[:id]) unless params[:id].blank?
    if @article.update_attributes(params[:article])
      redirect_to @article, :notice  => "Successfully updated article."
    else
      render :action => 'edit'
    end
  end

  def destroy
    ## Didn't test this, obviously no error management
    @article = Article.find(params[:id]) unless params[:id].blank?
    @article.destroy
    redirect_to articles_url, :notice => "Successfully destroyed article."
  end
end


Go to your router and verify or add:
 resources :articles
 match 'articles/rank/(:id)/(:process)' => "articles#rank", :as => :articles_rank
 


Your views in article(s) folder:
index.html.erb
<% "Articles" %>
<table >

  <% for article in @articles %>
    <tr>
    <%= content_tag_for :td, article do %>    ## Probably not necessary
     <td> <%= link_to "Move Up", articles_rank_url(article.id, "Move Up") %></td>
     <td> <%= link_to "Move Down", articles_rank_url(article.id, "Move Down") %></td>
     <td> <%= link_to "Move Top", articles_rank_url(article.id, "Move Top") %> </td>
     <td> <%= link_to "Move Bottom", articles_rank_url(article.id, "Move Bottom") %></td>
     <td> <%= link_to article.id.to_i, articles_rank_url(article.id, "Insert At") %></td>
     <td> <%= article.article %></td>
     <td> <%= "Position " + article.position.to_s %> </td>
     <td> <%= "Rank " + article.rank.to_s %> </td>
     <td> <%= link_to "Show", article %></td>
     <td> <%= link_to "Edit", edit_article_url(article.id) %> </td>
     <td> <%= link_to "Destroy", article, {:confirm => 'Are you sure?', :method => :delete} %></td>
    <% end %>
    </tr>
  <% end %>
</table>
<p><%= link_to "New Article", new_article_path, {:class=>"search"} %></p>

edit.html.erb
<%= "Edit Article" %>
<%= render 'form' %>
<p>
  <%= link_to "Show", @article %> |
  <%= link_to "View All", articles_path %>
</p>

_form.html.erb
<%= form_for @article do |f| %>
  <% if @article.errors.any? %>
  <% end %>
  <p>
    <%= f.label :article %><br />
    <%= f.text_field :article %>
  </p>
  <p><%= f.submit %></p>
<% end %>
new.html.erb
<%= "New Article" %>
<%= render 'form' %>
<p><%= link_to "Back to List", articles_path %></p>

show.html.erb

<%= "Article" %>

<p>
  <strong>Article:</strong>
  <%= @article.article %>
</p>
<p>
  <strong>postion:</strong>
  <%= @article.position %>
</p>

<p>
  <%= link_to "Edit", edit_article_path(@article) %> |
  <%= link_to "Destroy", @article, {:confirm => 'Are you sure?', :method => :delete} %> |
  <%= link_to "View All", articles_path %>
</p>

That's about it!  Hope it works for you!
The api (rails 4) source is : https://github.com/swanandp/acts_as_list

[Elizabeth McGurty]

CORRECTION:
Had a little fun with acts_as_list...

.

[Federicko]

Hi Colin,

I have done that but slightly differently as the 'this' variable was giving me an error.

Controller didn't change. It still just calls the rankup or rankdown methods in the model. 

Here's the code for that:

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

Then in my model I have 3 methods:

def rankup

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

  swap_rank(@affected_article)

end

def rankdown

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

  swap_rank(@affected_article)

end

private

def swap_rank(affected_article)

  @current_ranking = self.ranking

  self.ranking = affected_article.ranking

  affected_article.ranking = @current_ranking

  Article.transaction do

    self.save

    affected_article.save

  end

end

This way all the code sits in one method and there is no repeated code

[Federicko]

Thanks for the detailed post.

I will definitely check out the gem

[Colin Law]

On 4 July 2015 at 03:40, Federicko <fedo...@gmail.com> wrote:
> Hi Colin,
>
> I have done that but slightly differently as the 'this' variable was giving
> me an error.

Sorry it should have been 'self' not 'this' of course, as I am sure
you worked out. ruby != c++

That looks good, though you are still not handling the error
conditions on :rank and if either of the saves fail. No doubt you
plan to sort those. Don't forget to write automated tests to check
all the methods including the failure conditions.

Why do you need ranking.to_i-1 rather than ranking-1?

Colin