Topic: suggestion - code auditing by wisdom of the crowds
As the rails community so actively encourages good quality code, maybe it's an idea to have a section on the forum where one could post code to be audited by the community.
The purpose would NOT be to have suggestions on 'how' it could be fixed, refactored, or DRYed, but rather to point out the nature of the problems, the coventions that are being disregarded and why its bad, and to point out security holes and issues like lack of test coverage, untestability, unscalability, unreadability, unRESTfulness and so on. Maybe with some rating system to score those and other aspects of the code.
It seems like a process that would be better handled on a community level, where opinions and consensus on what constitutes bad code can evolve with the framework, rather than by any single body acting in isolation as an authority. And it would also give people a chance to see some of the nastiness that's out there and learn by example how not to do it. I just googled "fat controller example" and found just one at http://refactormycode.com/codes/253-fat … ler-method - which is absolutely anorexic compared to what (unnamed) outsource company just delivered to me. I pasted one controller action from them below. As you can guess, the model is inversely proportional in size.
I'm not looking for help to fix it, but it illustrates why a community code audit site/forum would help prevent this kind of thing from ever happening.
def artists
store_location
@usertype = Usertype.find_by_name('Employer')
if params[:search]
@users = User.find(:all,:conditions=>"usertype_id != 2 and (first_name like '%#{params[:search]}%' or last_name like '%#{params[:search]}%')",:order=>"first_name")
@allusers = User.find(:all,:conditions=>"usertype_id != 2")
skillusers = []
for user in @allusers
if user.skill_profile
for skill in user.skill_profile.skills
@skill = Skill.find(:all,:conditions => "name like '%#{params[:search]}%'")
if @skill
for myskill in @skill
if skill.name.downcase == myskill.name.downcase
skillusers << user.id
end
end
end
end
end
end
for otheruser in @users
skillusers << otheruser.id
end
if params[:search].include? ' '
@totalsearch = params[:search].split(' ')
for mysearch in @totalsearch
@fnameusers = User.find(:all,:conditions=>"usertype_id != 2 and (first_name like '%#{mysearch}%')",:order=>"first_name")
@lnameusers = User.find(:all,:conditions=>"usertype_id != 2 and (last_name like '%#{mysearch}%')",:order=>"first_name")
for user in @allusers
for skill in user.skill_profile.skills
@skill = Skill.find(:all,:conditions => "name like '%#{mysearch}%'")
if @skill
for myskill in @skill
if skill.name.downcase == myskill.name.downcase
skillusers << user.id
end
end
end
end
end
end
end
if @fnameusers != nil
for otheruser in @fnameusers
skillusers << otheruser.id
end
end
if @lnameusers != nil
for otheruser in @lnameusers
skillusers << otheruser.id
end
end
#@artists = User.find_all_by_id(skillusers)
@artists = User.paginate_by_id skillusers ,:page => params[:page],:order=>"first_name"
respond_to do |format|
format.html {render :layout => false }
end
elsif params[:advance]
conditions = "usertype_id != #{@usertype.id}"
conditions += " and first_name like '%#{params[:first_name]}%'" if params[:first_name] != ""
conditions += " and last_name like '%#{params[:last_name]}%'" if params[:last_name] != ""
@artist = User.find(:all,:conditions=>conditions,:order=>"first_name")
@artists = User.paginate :conditions => conditions,:page => params[:page],:order=>"first_name"
#flash[:notice] = @artists.length
#exit
startdate = params[:start_date].to_date if params[:start_date] != ""
enddate = params[:end_date].to_date if params[:end_date] != ""
if params[:start_date] != "" and params[:end_date] != ""
@new_ar = []
@temp = ""
for artist in @artist
if artist.diary
for busy in artist.diary.busy_periods
if ( startdate > busy.start_date and startdate < busy.end_date) or (enddate > busy.start_date and enddate < busy.end_date)
@temp = artist.id.to_s
break
end
end
if @temp != artist.id.to_s
@new_ar << artist.id
end
end
end
@artist = User.find_all_by_id(@new_ar,:conditions=>conditions,:order=>"first_name")
@artists = User.paginate_by_id @new_ar, :conditions => conditions,:page => params[:page],:order=>"first_name"
end
if params[:skills] != ""
@new_artist = []
@test = []
if params[:skills].include? ','
@skills = params[:skills].split(',')
else
@skills = []
@skills[0] = params[:skills]
end
for artist in @artist
if artist.skill_profile.skills
for ability in artist.skill_profile.skills
@test << ability
for skill in @skills
if ability.name.downcase == skill.downcase
@new_artist << artist.id
break
end
end
end
end
end
if @new_artist.length > 0
@artist = User.find_all_by_id(@new_artist,:conditions=>conditions,:order=>"first_name")
@artists = User.paginate_by_id @new_artist, :conditions => conditions,:page => params[:page],:order=>"first_name"
else
@artist = User.find_all_by_id(0,:conditions=>conditions,:order=>"first_name")
@artists = User.paginate_by_id 0, :conditions => conditions,:page => params[:page],:order=>"first_name"
end
end
if params[:project] != ""
@artist_pr = []
for artist in @artist
if artist.credit_profile.credits
for credit in artist.credit_profile.credits
if credit.project = params[:project]
@artist_pr << artist.id
end
end
end
end
if @artist_pr.length > 0
@artist = User.find_all_by_id(@artist_pr,:conditions=>conditions,:order=>"first_name")
@artists = User.paginate_by_id @artist_pr, :conditions => conditions,:page => params[:page],:order=>"first_name"
end
end
location_conditions = "country_id != 0"
location_conditions += " and can_commute = #{params[:can_commute]}" if params[:can_commute] != ""
location_conditions += " and can_relocate = #{params[:relocate]}" if params[:relocate] != ""
location_conditions += " and can_work_remotely = #{params[:work_remote]}" if params[:work_remote] != ""
location_conditions += " and commute_town = '#{params[:city]}'" if params[:city] != ""
location_conditions += " and country_id = #{params[:country][:name]}" if params[:country][:name] != ""
@workprofile = WorkProfile.find_all_by_user_id(@artist,:conditions=>location_conditions,:select=>"user_id as id")
@artist = User.find_all_by_id(@workprofile,:conditions=>conditions,:order=>"first_name")
@artists = User.paginate_by_id @workprofile, :conditions => conditions,:page => params[:page],:order=>"first_name"
# flash[:artist] = params[:platform_ids].length
# exit
if params[:platform_ids]
@users = []
for artist in @artist
for artistplatform in artist.work_profile.platforms_work_profiles
platform_condition = "work_profile_id = #{artist.work_profile.id}"
@j = 1
if params[:platform_ids].length > 0
platform_condition += " and platform_id IN ("
Platform.find(:all).length.times do
if params[:platform_ids][@j.to_s]
platform_condition += "#{params[:platform_ids][@j.to_s].to_i},"
end
@j += 1
end
platform_condition += "0)"
end
@platforms = PlatformsWorkProfile.find(:all,:conditions=>platform_condition)
#flash[:notice] = @platforms
#exit
if @platforms and @platforms.length == params[:platform_ids].length
@users << artist
end
#@j = 1
#Platform.find(:all).length.times do
# if params[:platform_ids][@j.to_s] == artistplatform.platform_id.to_s
# @users << artist.id
# break
# end
# @j += 1
#end
end
end
#@artists = User.find_all_by_id(@users,:conditions=>conditions,:order=>"first_name")
@artists = User.paginate_by_id @users, :conditions => conditions,:page => params[:page],:order=>"first_name"
end
# flash[:test] = location_conditions
# flash[:platforms] = @platforms
# flash[:notice] = @artists
#exit
else
#@artists = User.find(:all,:conditions => "usertype_id != '#{@usertype.id}'",:order=>"first_name")
@artists = User.paginate :conditions => "usertype_id != '#{@usertype.id}'",:page => params[:page],:order=>"first_name"
#Project.paginate_by_organization_id params[:organization_id], :order => 'id'
end
end