Refactor two similar actions

Rails
1

This code works but looks ugly.
How to merge this two actions?

#verifications/index.haml

  • if Ad.unverified_ads.size == 0
  • else
    = link_to “Szybka Weryfikacja”,
    fast_verify_info_verification_path(Ad.unverified_ads.last), :method
    => :put
    = link_to “Normal Verify”, verify_info_verification_path(ad), :method
    => :put

#verifications/verify_info.haml
= link_to “Verify”, verify_verification_path(@ad)
#verifications/fast_verify_info.haml
= link_to “Weryfikuj”,
fast_verify_verification_path(Ad.unverified_ads.last), :method => :put
#verification_controller.rb
def verify_info
@ad = Ad.find(params[:id])
end
def fast_verify_info
@ad = Ad.find(params[:id])
end

def fast_verify
@ad = Ad.find(params[:id])
@ad.verify!
if Ad.unverified_ads.size == 0
redirect_to verifications_path
else
redirect_to
fast_verify_info_verification_path(Ad.unverified_ads.last)
end
end
def verify
@ad = Ad.find(params[:id])
@ad.verify!
if params[:fast] == true
redirect_to Ad.unverified_ads.last
else
redirect_to verifications_path
end
end

2

Darek F. wrote in post #1042021:

This code works but looks ugly.
How to merge this two actions?

#verifications/index.haml

  • if Ad.unverified_ads.size == 0
  • else
    = link_to “Szybka Weryfikacja”,
    fast_verify_info_verification_path(Ad.unverified_ads.last), :method
    => :put

Ad.unverified_ads.last
The work of loading the last unverified ad should not be performed
directly by the view. It’s not the views responsibility. Besides that,
this line is a direct violation if the Law of Demeter (LoD). While, I
only partially support the concept of LoD, doing this in the view is
really crossing that line.

= link_to “Normal Verify”, verify_info_verification_path(ad), :method
=> :put

#verifications/verify_info.haml
= link_to “Verify”, verify_verification_path(@ad)
#verifications/fast_verify_info.haml
= link_to “Weryfikuj”,
fast_verify_verification_path(Ad.unverified_ads.last), :method => :put
#verification_controller.rb
def verify_info
@ad = Ad.find(params[:id])
end
def fast_verify_info
@ad = Ad.find(params[:id])
end

I don’t understand the purpose of these two methods. They find an ad and
do nothing else. Seems rather pointless to me. I could be missing
something here.

If you just need to have the same ad loaded in more than one action
method
then use a before filter to load it.

before_filter :load_ad

private
def load_ad
@ad = Ad.find(params[:id])
end

See

for more on controller filters.

def fast_verify
@ad = Ad.find(params[:id])
@ad.verify!
if Ad.unverified_ads.size == 0
redirect_to verifications_path
else
redirect_to
fast_verify_info_verification_path(Ad.unverified_ads.last)
end
end
def verify
@ad = Ad.find(params[:id])
@ad.verify!
if params[:fast] == true
redirect_to Ad.unverified_ads.last
else
redirect_to verifications_path
end
end

Again I see Ad.unverified_ads.last. If this is important enough to keep
restating multiple times then it’s important enough for your Ad model to
know how to do it.

Ad.last_unverified (This helps resolve the LoD issue).