Newbie code review

rubyonrails-talk
21

CORRECTION: Had a little fun with acts_as_list…

22

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

23

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

24

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

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

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