Upgrade to Pro — share decks privately, control downloads, hide ads and more …

RubyConf AU 2015: Static Analysis Driven Refactoring

RubyConf AU 2015: Static Analysis Driven Refactoring

We'll illustrate how we can use static analysis tools to drive the refactoring of a Rails app.

First, we'll quickly introduce static analysis, its benefits and complimentarity with other practices. We'll present each tools, how to understand them, and how they can drive the refactoring on a real big open source project. We'll show how to automate their run and integrate it in your workflow. Finally, if we get the time, we'll do the same on projects of the audience. http://lanyrd.com/2015/rubyconf-au/sdgqxr/

Christophe Philemotte

February 04, 2015
Tweet

More Decks by Christophe Philemotte

Other Decks in Programming

Transcript

  1. $ man ruby -----8<--------------- -c Causes Ruby to check the

    syntax of the script and exit without executing. If there are no syntax errors, Ruby will print “Syntax OK” to the standard output. -----8<--------------- -w Enables verbose mode without printing version message at the beginning. It sets the $VERBOSE variable to true. -----8<---------------
  2. $ # Get all Ruby warnings $ grep rb_warn parse.y

    | > sed -e "/# define/d" | > sed -e "s/^[^\"]*\"//" | > sed -e "s/\"[^\"]*$//"
  3. else without rescue is useless END in method; use at_exit

    ambiguous first argument; put parentheses or even spaces `"op"' after local variable or literal is interpreted as binary operator even though it seems like "syn" Float %s out of range invalid character syntax; use ?\\%c `**' interpreted as argument prefix `*' interpreted as argument prefix `&' interpreted as argument prefix shadowing outer local variable - %s empty expression possibly useless use of %s in void context string literal in condition assigned but unused variable - %s named capture conflicts a local variable - %s
  4. ➔ cpu & mem profiling ➔ call graphs ➔ code

    coverage ➔ functionality / test ➔ race condition / deadlock ➔ dead code
  5. ➔ code smells ➔ flaws & defects ➔ vulnerabilities ➔

    poor readability ➔ poor design ➔ poor documentation ➔ metrics // high risk of bugs
  6. ➔ it's a program ◆ indefatigable ◆ reliable ◆ reproducible

    ◆ focus and precise ◆ automation ➔ it drives refactoring & quality ➔ it saves time
  7. ➔ it's a program ◆ false positives ◆ cannot grasp

    business logic ◆ limited ➔ it needs setup & maintenance
  8. ➔ code review is done by a human ➔ tests

    are about a priori behaviors ➔ quality is important
  9. ➔ Worst case: better product for same effort ➔ Best

    case: better product for less effort ➔ 2 x → 100 x less expensive
  10. # Check setup is ok $ RAILS_ENV=test bundle exec rake

    db: create db:migrate db:seed_fu $ bundle exec rake spec:models $ bundle exec rake spec:controllers $ bundle exec rake spec:mailers $ bundle exec rake spec:requests $ bundle exec rake spec:helpers
  11. ➔ Flay ➔ Excellent ➔ Brakeman ➔ Rubocop ➔ Rails

    Best Practices ➔ Yard / inch ➔ Reek
  12. $ flay app > reports/flay_app.out $ cat reports/flay_app.out $ #

    What do you notice? $ # Let's take a look at $ # - dashboard_controller.rb $ # - groups_controller.rb
  13. # ... def merge_requests @merge_requests = MergeRequestsFinder.new.execute(current_user, params) @merge_requests =

    @merge_requests.page(params[:page]).per(20) @merge_requests = @merge_requests.preload(:author, :target_project) end # ...
  14. # ... def issues @issues = IssuesFinder.new.execute(current_user, params) @issues =

    @issues.page(params[:page]).per(20) @issues = @issues.preload(:author, :project) respond_to do |format| format.html format.atom { render layout: false } end end # ...
  15. # dashboard_base_controller.rb class DashboardBaseController < ApplicationController def merge_requests @merge_requests =

    MergeRequestsFinder.new.execute(current_user, params) @merge_requests = @merge_requests.page(params[:page]).per(20) @merge_requests = @merge_requests.preload(:author, :target_project) end def issues @issues = IssuesFinder.new.execute(current_user, params) @issues = @issues.page(params[:page]).per(20) @issues = @issues.preload(:author, :project) respond_to do |format| format.html format.atom { render layout: false } end end end
  16. $ gem install excellent $ excellent --help Usage: excellent [OPTIONS]

    <PATH1> [<PATH2> ...] … $ # Read the help $ excellent --checks $ # Look at the list
  17. Code Smells: • Bad Practices • Actual Error • Could

    be an error • Could lead to an error
  18. # app/mailers/emails/projects.rb#19 # it has abc score of 17.03 #

    ... def repository_push_email(project_id, recipient, author_id, branch, compare) # ...
  19. |ABC| = sqrt( A * A + B * B

    + C * C ) A = Assignment B = Branch C = Condition
  20. # app/workers/post_receive.rb#7 # it has flog score of 32. #

    ... def perform(repo_path, oldrev, newrev, ref, identifier) # ...
  21. Flog = ABC for Ruby C = Calls + weight,

    e.g. metaprogramming https://github.com/seattlerb/flog/blob/master/lib/flog.rb#L13-67
  22. Good ABC/Flog Score? 0-10 Awesome 11-20 Good enough 21-40 Might

    need refactoring 41-60 Possible to justify 61+ Danger
  23. # app/helpers/commits_helper.rb#107 # it has cyclomatic complexity of 18. #

    ... def parallel_diff_lines(project, commit, diff, file) # ...
  24. # app/mailers/emails/projects.rb#19 # it has 5 parameters # ... def

    repository_push_email(project_id, recipient, author_id, branch, compare) # ...
  25. $ gem install brakeman $ brakeman --help Usage: brakeman [options]

    rails/root/path … $ # Read the help $ brakeman --checks $ # Look at the list
  26. $ brakeman . -o reports/brakeman.out $ # Look at the

    report $ brakeman . -o reports/brakeman.html $ firefox reports/brakeman.html $ # Look at the report
  27. # app/controllers/projects/ # team_members_controller.rb#22 # Unvalidated Redirect def create #

    … if params[:redirect_to] redirect_to params[:redirect_to] # …
  28. # Gemfile#11 # Rails 4.1.1 contains a SQL # injection

    vulnerability # (CVE-2014-3482). Upgrade to 4.1.3 # … gem "rails", "~> 4.1.0" # …
  29. $ gem install rubocop $ rubocop --help Usage: rubocop [options]

    [file1, file2, ...] … $ # Read the help
  30. == app/mailers/notify.rb == C: 1: 1: Missing top-level class documentation

    comment. C: 15: 81: Line is too long. [106/80] C: 18: 17: Use proc instead of Proc.new. C: 31: 28: Prefer single-quoted strings when you don't need string interpolation or special symbols. C: 36: 81: Line is too long. [81/80] C: 39: 5: Use a guard clause instead of wrapping the code inside a conditional expression. C: 52: 5: Use a guard clause instead of wrapping the code inside a conditional expression. W: 52: 18: Assignment in condition - you probably meant to use ==. C: 61: 7: Do not prefix writer method names with set_. C: 62: 13: Prefer single-quoted strings when you don't need string interpolation or special symbols. C: 69: 7: Do not prefix writer method names with set_. C: 70: 13: Prefer single-quoted strings when you don't need string interpolation or special symbols. C: 92: 15: Prefer single-quoted strings when you don't need string interpolation or special symbols. ...
  31. == app/mailers/notify.rb == C: 16: 81: Line is too long.

    [106/80] C: 32: 28: Prefer single-quoted strings when you don't need string interpolation or special symbols. C: 65: 13: Prefer single-quoted strings when you don't need string interpolation or special symbols. C: 73: 13: Prefer single-quoted strings when you don't need string interpolation or special symbols. C: 95: 15: Prefer single-quoted strings when you don't need string interpolation or special symbols.
  32. Style Refactoring Basics: • No Truth, Just Convention • Read

    by Humans not Machine • Do not Surprise you and others
  33. $ # Uncheck All $ # 1 Always Add Db

    Index $ # db/schema.rb#68
  34. $ # Uncheck All $ # 1 Always Add Db

    Index $ # db/schema.rb#68 $ RAILS_ENV=test rails generate migration AddIndexToForkedProjectLinks $ RAILS_ENV=test rake db:migrate
  35. $ # 2 Use Say With Time In Migrations $

    # db/migrate/ 20140416074002_add_index_on_iid.rb
  36. $ # 2 Use Say With Time In Migrations $

    # db/migrate/ 20140416074002_add_index_on_iid.rb $ RAILS_ENV=test rake db:drop db: create db:migrate db:seed_fu
  37. $ # 4 Replace Instance Variable With Local Variable $

    # app/views/projects/tree/ _tree_item.html.haml
  38. $ # 4 Replace Instance Variable With Local Variable $

    # app/views/projects/tree/ _tree_item.html.haml#5 $ # app/helpers/tree_helper.rb#13
  39. Sandi Metz' 4 Rules: • Keep class short < 100

    lines • Keep method short < 5 lines • Don't pass too many parameters < 4 • Use 1 instance var per controller • "You should break these rules only if you have a good reason"
  40. $ gem install yard $ yard stats --list-undoc > reports/yard.out

    $ # app/controllers/admin/ background_jobs_controller.rb
  41. Documentation: • Code is not Doc • API is already

    Doc • Be Public & Understood • Explaining Business Domain • About Details • How to use the Code • How to understand the code
  42. $ gem install inch $ inch suggest --all app/ >

    reports/inch.out $ cat reports/inch.out
  43. $ gem install reek $ reek -S app/ > reports/reek.txt

    $ # app/helpers/commits_helper.rb
  44. # lib/task/review.rake desc "Review done by Static Analysis Tools" task

    :review do |variable| `flay app > reports/flay_app.out` `excellent app lib -o reports/excellent.html` `brakeman . -o reports/brakeman.html` `rubocop app -f o -o reports/rubocop_summary.out` `rubocop app -f s -o reports/rubocop.out` `rails_best_practices -f html --output-file reports/rails_best_practices.html` `yard stats --list-undoc > reports/yard.out` `inch suggest --all app > reports/inch.out` end
  45. # Gemfile group :development, :test do # ... gem 'flay',

    require: false gem 'excellent', require: false gem 'rubocop', require: false gem 'brakeman', '2.6.1', require: false gem 'yard', require: false gem 'inch', require: false # gem 'rails_best_practices' already there end
  46. ➔ "Agile", step by step ➔ 1st: 1 tool &

    locally ➔ 2nd: show off ➔ 3rd: CI ➔ 4th: reapeat