flyerhzm / rails_best_practices

a code metric tool for rails projects
http://rails-bestpractices.com
MIT License
4.17k stars 276 forks source link

False positives for controllers with inheritance #181

Open lstrzebinczyk opened 11 years ago

lstrzebinczyk commented 11 years ago

I have such code in my applications admin panel:

module Admin
  class AppController < ApplicationController
    def index
      @collection = model.all
    end
  end
end

module Admin
  class UsersController < AppController
    private

    def model
      User
    end
  end
end

Actual code is a little bit bigger, but example above shows the general rule. I'm receiving false positives with unused methods and restricting routes:

routes.rb:7 - restrict auto-generated routes admin/users (only: [])
users_controller.rb:43 - remove unused methods (Admin::UsersController#model)

It should probably find methods used in ancestor controllers. If this is true, I'll try to fix it myself.

flyerhzm commented 11 years ago

@KillaPL this is probably something wrong I did in lib/rails_best_practices/prepares/controller_prepare.rb

flyerhzm commented 11 years ago

@KillaPL I think I have already fixed it on master branch, please check it out and let me know if it works for you.

lstrzebinczyk commented 11 years ago

The amount of false positives in routes didn't change. The amount of catches in unused methods did drop, but it finds new false positives now.

This is the case for some of them:

controllers/admin/foos_controller.rb:

module Admin
  class FoosController < AppController
    private

    def bar
    end
  end
end

views/admin/app/new.html.erb:

<%= render 'partial' %>

views/admin/foos/_partial.html.erb:

<%= bar %>

Baically, controller renders view from parent's controller directory, inside which it renders partial from it's own directory.

I also have one case where <%= bar %> would be in new.html.erb, instead of rendering a partial.

One thing that's interesting is

module Api
  class JobsController < ApplicationController
    def update_multiple
    end
  end
end

Because in my roots, I have

namespace :api do
  resources :jobs do
    collection do
      patch :update_multiple
    end
  end
end

A couple of models get's false positives as unused methods, which for some reason are listed as ActionView::Helpers :

models/job.rb:38 - remove unused methods (ActionView::Helpers::Job#available_labels)
models/job.rb:44 - remove unused methods (ActionView::Helpers::Job#label_proper?)
models/job.rb:52 - remove unused methods (ActionView::Helpers::Job#create_colors)
models/job.rb:58 - remove unused methods (ActionView::Helpers::Job#set_stage_as_start)
models/job.rb:62 - remove unused methods (ActionView::Helpers::Job#update_system_id)
models/job.rb:66 - remove unused methods (ActionView::Helpers::Job#stages_to_labels_map)

First one is used like this:

model:

def available_labels
  stages_to_labels_map[stage]
end

controller:

def common_labels
  foos.map(&:available_labels).inject(:&)
end

second one is used in validation:

validate  :label_proper?

def label_proper?
end

3-5 are used as callbacks:

before_create :set_stage_as_start
after_create :update_system_id
after_initialize :create_colors

6th is used in regular way in #available_labels above

Hope I helped : )