Newbie code review

Rails
21

On 2 July 2015 at 12:04, Federicko [email protected] wrote:

@this_article.rankdown

self.ranking = self.ranking - 1
@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

22

CORRECTION:
Had a little fun with acts_as_list…

23

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

<% for article in @articles %>


<%= content_tag_for :td, article do %> ## Probably not necessary











<% end %>

<% end %>

<%= link_to “Move Up”, articles_rank_url(article.id, “Move
Up”)
%>		 <%= link_to “Move Down”, articles_rank_url(article.id, “Move
Down”) %>		 <%= link_to “Move Top”, articles_rank_url(article.id, “Move
Top”)
%> 		<%= link_to “Move Bottom”, articles_rank_url(article.id, “Move
Bottom”) %>		 <%= link_to article.id.to_i, articles_rank_url(article.id,
“Insert At”) %>		 <%= article.article %> <%= "Position " + article.position.to_s %> <%= "Rank " + article.rank.to_s %> <%= link_to “Show”, article %> <%= link_to “Edit”, edit_article_url(article.id) %> <%= link_to “Destroy”, article, {:confirm => ‘Are you sure?’,
:method => :delete} %>

<%= link_to "New Article", new_article_path, {:class=>"search"} %>

edit.html.erb
<%= “Edit Article” %>
<%= render ‘form’ %>

<%= link_to "Show", @article %> | <%= link_to "View All", articles_path %>

_form.html.erb
<%= form_for @article do |f| %>
<% if @article.errors.any? %>
<% end %>

<%= f.label :article %>
<%= f.text_field :article %>

<%= f.submit %>

<% end %> new.html.erb <%= "New Article" %> <%= render 'form' %>

<%= link_to "Back to List", articles_path %>

show.html.erb

<%= “Article” %>

Article: <%= @article.article %>

postion: <%= @article.position %>

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

That’s about it! Hope it works for you!
The api (rails 4) source is : GitHub - brendon/acts_as_list: An ActiveRecord plugin for managing lists.

24

Thanks for the detailed post.
I will definitely check out the gem

25

On 4 July 2015 at 03:40, Federicko [email protected] 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++

else
@affected_article = Article.find_by(ranking: ranking.to_i-1)
end
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

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

26

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