2
\$\begingroup\$

My rating controller:

  def rate
    @ip = request.remote_ip
    @konkurrencer = Rating.where(:IP => @ip, :konkurrencer_id => params[:id])
    if @konkurrencer.empty?
      params[:kon][:IP] = request.remote_ip
      params[:kon][:konkurrencer_id] = params[:id]
      @konkurrencer = Rating.new(params[:kon])
      @konkurrencer.save
      @konkurrencer.konkurrencer.rating_score += params[:kon][:ratings].to_i
      @konkurrencer.konkurrencer.ratings += 1
      @konkurrencer.save
    else
      // render error message
    end
    render :nothing => true
   end
end

My AJAX:

 jQuery.ajax({
     url: frm.attr('action'), //your server side script
     data: frm.serialize(), //our data
     type: 'POST',
     success: function (data) {
          //write here something when everything went ok

     },
     error: function (jxhr, msg, err) {
         alert(msg); //something went wrong.
     }
 });

Have do I make my controller more DRY?

\$\endgroup\$
  • \$\begingroup\$ I can't help but wonder if it would be possible to mess up your rating system by using Firebug to manipulate the values being sent in the request. \$\endgroup\$ – ANeves thinks SE is evil Feb 27 '12 at 17:30
1
\$\begingroup\$

I would try and move code up into the model for a 'fat model, thin controller' approach. You want your business logic to be in the model whenever possible. This is a DRY approach that reduces duplication and makes testing easier.

The find: @konkurrencer = Rating.where(:IP => @ip, :konkurrencer_id => params[:id]) should be a model finder, maybe a model method called 'konkcurrent'

def koncurrent
  where(:IP => params[:id], :konkurrencer_id => params[:id]
end

Also this has a repeat:

  @konkurrencer.save
  @konkurrencer.konkurrencer.rating_score += params[:kon][:ratings].to_i
  @konkurrencer.konkurrencer.ratings += 1
  @konkurrencer.save

so I would remove the first .save I would also look to transfer this to the model, you could have a model method called 'increment_rating_score'

Add model testing for both of these.

Please note that symtax is aimed at rails3 and code is pseudo code to show approach, syntax might need cleaning up.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Controller code looks good.. You only need to refactor code little bit so that it will look more clean & robust. otherwise rest things are ok. regarding ajax response, you need to implement success/failure response either inside controller or js file.

just to give idea on implementing ajax response inside controller

def update
  render :update do |page|
    if success
      # here replace_html is prototype helper method
      page.replace_html "DIV_ID", "PARTIAL"
      # if you are using jquery then you can execute js code like
      page << "js code goes here"
    else
      page.replace_html "ERR_DIV", "Error occured"
    end
  end
end
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ How do I implement success/failure response in the controller_ \$\endgroup\$ – Rails beginner Feb 26 '12 at 13:44
  • \$\begingroup\$ edited answer to give you brief idea .. \$\endgroup\$ – Sandip Ransing Feb 26 '12 at 18:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.