Brzydki kod

piotrstanek · February 18, 2013, 3:24pm

Hej,

Można poprawić coś takiego:

[code]class VcardController < ApplicationController
layout “vcard”

def index
if params[:card_id] != nil
@card = AppCardResult.find_by_hash_id(params[:card_id].to_i)
else
redirect_to app_list_path
end
end

private
def isset_card_to_user
if params[:card_id] != nil
if AppCardResult.where(:hash_id => params[:card_id].to_i).count == 0
redirect_to app_list_path
end
end
end
end[/code]
JEst napisane dobrze czy zawsze muszę dawać to_i albo to_s itp…

drogus · February 18, 2013, 3:33pm

Nie musisz dawać to_i, bo ActiveRecord sam to zrobi jeżeli kolumna jest typu liczbowego. Tak samo nie musisz dawać params[:card_id] != nil, wystartczy:

[code]if params[:card_id]

end[/code]

krzyzak · February 18, 2013, 3:33pm

hej,
szybki refactor z palca, może zawierać błędy:

[code=ruby]class VcardController < ApplicationController
before_filter :require_card_id
def index
@card = AppCardResult.find_by_hash_id(params[:card_id])
end

private
def require_card_id
redirect_to app_list_path unless params[:card_id]
end

def isset_card_to_user
redirect_to app_list_path if AppCardResult.where(hash_id: params[:card_id]).zero?
end
end[/code]

jesli w większej ilości miejsc potrzebujesz params[:card_id].to_i, przypisz sobie ją do zmiennej
nie sprawdzaj kilka razy tego samego (params[:card_id] != nil)
pisz w rubym! (używaj unless, .zero? itp)
jeśli pole jest intowe, to AR sam robi .to_i za Ciebie
moje wątpliwości budzi też nazewnictwo, plus to, jak korzystasz z całości tego kodu, ale to już byłaby zabawa w dłuższy refactor i wymagała znajomości większego kawałka kodu, także może to pomińmy

piotrstanek · February 18, 2013, 3:51pm

ok dzięki, to pytam dalej

mam takie coś w helperze można to napisać ładniej krócej

[code]module Client::FanpageHelper

def self.download_fanpages
@graph = Koala::Facebook::API.new(Client.find(session[:id_client]).access_token.to_s)
for facebook_page in @graph.get_object(“me/accounts/page”)
if !Fanpage.find_by_uid_and_user(facebook_page[“id”],session[:id_client].to_i)
@fb = Fanpage.new()
@fb.name = facebook_page[“name”].to_s
@fb.access_token = facebook_page[“access_token”].to_s
@fb.uid = facebook_page[“id”]
@fb.user = session[:id_client]

    #download url to fanpage
    @graphs = Koala::Facebook::API.new(facebook_page["access_token"]) 
    @url = @graphs.get_object("me") 
    @fb.url = @url['link']
    if @url['link'].index("facebook")
      @fb.save
    end
  else
    @fb = Fanpage.find_by_uid(facebook_page["id"])
    @fb.name = facebook_page["name"]
    @fb.access_token = facebook_page["access_token"]
    @fb.uid = facebook_page["id"]
    @fb.user = session[:id_client]
    #download url to fanpage
    @graphs = Koala::Facebook::API.new(facebook_page["access_token"]) 
    @url = @graphs.get_object("me") 
    @fb.url = @url['link']
    if @url['link'].index("facebook")
      @fb.save
    end
  end
end

end

end[/code]
PS. jak kolorować kod na forum ??

emka · February 18, 2013, 5:47pm

Zauwaz, ze to co masz w ifie jest praktycznie takie samo, poza jedna linia. @fb = … zostaw tak jak jest, to jest

if !Fanpage.find_by_uid_and_user(facebook_page["id"],session[:id_client].to_i) @fb = Fanpage.new() else @fb = Fanpage.find_by_uid(facebook_page["id"]) end
reszte wywal za ifa.

Kod kolorujesz dodajac ‘code=ruby’

piotrstanek · February 18, 2013, 7:00pm

ok dzięki zmodyfikowałem jeszcze dodatkowo jedno IF’a

[code=ruby]module Client::FanpageHelper

def self.download_fanpages(user_id)
#API Facebook
@graph = Koala::Facebook::API.new(Client.find(user_id).access_token.to_s)

#download list of fanpage
for facebook_page in @graph.get_object("me/accounts/page")
  @graphs = Koala::Facebook::API.new(facebook_page["access_token"]) 
  @url = @graphs.get_object("me") 
  
  if @url['link'].index("facebook")    
    if !Fanpage.find_by_uid_and_user(facebook_page["id"],user_id)
      @fb = Fanpage.new()
    else
      @fb = Fanpage.find_by_uid(facebook_page["id"])
    end
    @fb.name = facebook_page["name"]
    @fb.access_token = facebook_page["access_token"]
    @fb.uid = facebook_page["id"]
    @fb.user = user_id
    @fb.url = @url['link']
    @fb.save
  end
end

end

end[/code]

marr · February 18, 2013, 8:24pm

Fanpage.find_by_uid_and_user(facebook_page["id"],user_id) Fanpage.find_by_uid(facebook_page["id"])
jeśli powyższe procedury zwracają to samo to można jeszcze uprościć

można też wydzielić kod z tworzeniem Fanpage aż do jego zapisu. Może to być funkcja w obecnym helperze lub metoda klasy Fanpage,
przykładowe użycie @fb.import_attributes(name = facebook_page["name"], access_token = facebook_page["access_token"], ..., url = @url['link'])

piotrstanek · February 20, 2013, 4:43pm

mam jeszcze jedno pytanie

można zrobić coś z takim czymś

[code=ruby]class App < ActiveRecord::Base
attr_accessible :active, :name, :page_id, :user_id, :app_type, :block, :html
validates :name, :length => { :in => 3…40 }
validates_presence_of :page_id
has_one :applications_typ, :primary_key=>:app_type, :foreign_key=>:id
has_one :fanpage, :primary_key=>:page_id, :foreign_key=>:id
has_one :app_config, :dependent => :destroy
has_many :app_progress, :dependent => :destroy
has_one :app_detal, :dependent => :destroy, :primary_key=>:id, :foreign_key=>:app_id
has_many :app_look, :dependent => :destroy
has_many :app_wall, :dependent => :destroy
has_many :app_obligation_like_pages, :dependent => :destroy
has_many :app_puzzle_imagess, :dependent => :destroy
has_many :app_puzzle_resultss, :dependent => :destroy
has_many :app_text, :dependent => :destroy
has_many :invite, :dependent => :destroy
has_many :app_text_resultss, :dependent => :destroy
has_many :app_image_resultss, :dependent => :destroy
has_many :app_image, :dependent => :destroy
has_many :app_cards, :dependent => :destroy
has_many :app_cards_resultss, :dependent => :destroy

has_many :app_users, :dependent => :destroy
has_many :view, :dependent => :destroy
has_many :app_ranking, :dependent => :destroy
has_many :app_draw, :dependent => :destroy

has_many :app_obligation_like_page, :dependent => :destroy
has_one :client, :primary_key=>:user_id, :foreign_key=>:id
has_one :app_active, :dependent => :destroy, :primary_key=>:id, :foreign_key=>:app_id
after_create :after_create

def after_create
#add progres
AppProgres.insert_progres self.id, ‘config_step_1’, 10
#add app config
app_config = AppConfig.AppDetal.AppLook.AppWall.AppActive.new :app_id => self.id
app_config.save
#add app detal
app_config = AppDetal.new :app_id => self.id
app_config.save
#add app look
app_look = AppLook.new :app_id => self.id
app_look.save
#add app wall
app_wall = AppWall.new :app_id => self.id
app_wall.save
#add app active
app_active = AppActive.new :app_id => self.id
app_active.save
end

def self.list(page_id)
App.find(
:all,
:joins => [:app_active,:app_detal],
:select => ‘apps.app_type as app_type, app_detals.html as html, apps.id as id, apps.name as name, app_actives.active as active,app_detals.date_start as date_start, app_detals.date_see as date_see,app_detals.time_start as time_start’,
:conditions => [“block = ‘0’ and active = ‘1’ and page_id = '”+page_id.to_s+"’ and CONCAT(date_start,’ ',time_start) <= ‘"+Time.now.strftime("%Y-%m-%d %H:%M")+"’ and date_see >= ‘"+Time.now.strftime("%Y-%m-%d")+"’ "]
)
end

end[/code]

marr · February 20, 2013, 5:15pm

Można zredukować podobnie do

has_many [:app_progress, :app_look .....  :app_cards_resultss], :dependent => :destroy

Czy tu “app_cards_resultss”, “app_puzzle_imagess” jest o jedno “s” za dużo ?
A tu “AppProgres” o jedno “s” za mało ?

piotrstanek · February 20, 2013, 5:47pm

“Czy tu “app_cards_resultss”, “app_puzzle_imagess” jest o jedno “s” za dużo ?
A tu “AppProgres” o jedno “s” za mało ?” jest ok …

konole · February 20, 2013, 8:43pm

Jak dla mnie, z tymi has_many, has_one - pogrupuj najpierw względem has_one, potem has_many, a dla każdej z tych kategorii posegreguj je alfabetycznie - łatwiej potem będzie Ci się w tym odnaleźć - ewentualnie tematycznie, jeśli podzielone jest to na jakieś sensowne bloki modeli, które są ze sobą powiązane.