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