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.

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.

Start by creating a new rails app

Once that loads properly, go to your GemFile and add:
gem “nifty-generators”, :group => :development ##
https://github.com/ryanb/nifty-generators
run bundle install
Hopefully that worked…

{For some reason I can’t expand my text here, ending abruptly}

On 28 June 2015 at 18:01, Federicko [email protected] wrote:

http://guides.rubyonrails.org/getting_started.html
end
@this_article.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

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

On Sunday, June 28, 2015, Federicko [email protected] 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

On 28 June 2015 at 22:15, Elizabeth McGurty [email protected] 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

Hi All,

Thank for the feedback.

Yeah, my next step is to consolidate the two actions together.

Good point on using POST instead of GET.
I will change the link_to helper.

As to using generators, I followed the tutorial and used generators for
creating the model and controllers for Articles and Comments.

Cheers

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.

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

On 29 June 2015 at 19:42, Federicko [email protected] wrote:

def rank
@this_article.rankdown
are ranking up or down.

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.

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

Colin

On 29 June 2015 at 21:27, Federicko [email protected] wrote:

def self.rank(id, rank)
@this_article.rankdown
self.rank = self.rank + 1
end
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

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

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

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.

On 1 July 2015 at 05:06, Federicko [email protected] wrote:

6 Programming is fun 2

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

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

On 1 July 2015 at 09:33, Federicko [email protected] wrote:

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

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

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

On 2 July 2015 at 11:07, Federicko [email protected] wrote:

else
def rankup(rank)
self.save
@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.

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

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.

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

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs