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/

3f8fcddf7ab5d1bd90b0a0a9adfd6527?s=128

Christophe Philemotte

February 04, 2015
Tweet

Transcript

  1. Static Analysis Driven Refactoring Christophe Philemotte, 2015-02-04, @rubyconf_au 2015

  2. _toch toch Hi, I'm Christophe

  3. Belgium

  4. Belgium

  5. Co-founder of

  6. None
  7. Code = Garden

  8. None
  9. How?

  10. None
  11. → Every Day → Equipped → Skilled

  12. Outline

  13. → What & why? → Toolbelt → SDLC & Auto

  14. Workshop Dynamics?

  15. Please Interact!

  16. Please Interact!!

  17. Please Interact!!!

  18. What & Why?

  19. What's Static Analysis?

  20. Analysis of your software without executing it

  21. == Code Review

  22. == Code Review but with a program

  23. Code Read IR Analyze Results

  24. # stupid.rb var = 1

  25. $ 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<---------------
  26. $ ruby -wc stupid.rb stupid.rb:1: warning: assigned but unused variable

    - test Syntax OK
  27. $ # Get all Ruby warnings $ grep rb_warn parse.y

    | > sed -e "/# define/d" | > sed -e "s/^[^\"]*\"//" | > sed -e "s/\"[^\"]*$//"
  28. 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
  29. What cannot be detected?

  30. What needs to be executed

  31. ➔ cpu & mem profiling ➔ call graphs ➔ code

    coverage ➔ functionality / test ➔ race condition / deadlock ➔ dead code
  32. What can be detected?

  33. ➔ code smells ➔ flaws & defects ➔ vulnerabilities ➔

    poor readability ➔ poor design ➔ poor documentation ➔ metrics // high risk of bugs
  34. What are their benefits?

  35. ➔ it's a program ◆ indefatigable ◆ reliable ◆ reproducible

    ◆ focus and precise ◆ automation ➔ it drives refactoring & quality ➔ it saves time
  36. What are their drawbacks?

  37. ➔ it's a program ◆ false positives ◆ cannot grasp

    business logic ◆ limited ➔ it needs setup & maintenance
  38. Why should we use it?

  39. ➔ code review is done by a human ➔ tests

    are about a priori behaviors ➔ quality is important
  40. Why quality is important?

  41. " Code is like farts. It stinks if it isn't

    yours." Anonymous
  42. When you start, you build a kart.

  43. None
  44. Eventually, you'll end with a Boïng.

  45. None
  46. Inertia is not the same

  47. ➔ Worst case: better product for same effort ➔ Best

    case: better product for less effort ➔ 2 x → 100 x less expensive
  48. None
  49. None
  50. What's refactoring?

  51. ➔ Restructuring existing Without changing behavior ➔ Improves structural quality

  52. The Toolbelt

  53. Setup The project

  54. GitLab Setup Instructions ➔ Big ➔ I don't know ➔

    Open Source ➔ Rails app
  55. # 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
  56. $ mkdir reports

  57. ➔ Flay ➔ Excellent ➔ Brakeman ➔ Rubocop ➔ Rails

    Best Practices ➔ Yard / inch ➔ Reek
  58. Flay Duplication

  59. URI: https://github.com/seattlerb/flay Purpose: Spot duplicated code

  60. $ gem install flay $ flay --help flay [options] files_or_dirs

    … $ # Read the help
  61. $ flay . > reports/flay.out $ cat reports/flay.out $ #

    What do you notice?
  62. $ flay -s . > reports/flay_summary.out $ cat reports/flay_summary.out $

    # Where do dups occur the most?
  63. $ flay app > reports/flay_app.out $ cat reports/flay_app.out $ #

    What do you notice?
  64. $ 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
  65. # ... 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 # ...
  66. # ... 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 # ...
  67. # 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
  68. $ flay app > reports/flay_app.out $ cat reports/flay_app.out $ #

    Dups are removed \o/
  69. Dup Refactoring: • app/mailers/emails/notes.rb • app/controllers/{dashboard,groups} _controller.rb • app/services/{issues,merge_requests} /update_service.rb

  70. Dup Refactoring: • Extract Method • Pull-Up • Strategy or

    Mixin
  71. Excellent Code Smells

  72. URI: https://github. com/simplabs/excellent Purpose: Spot Code Smells https://github. com/simplabs/excellent/wiki#checks

  73. $ gem install excellent $ excellent --help Usage: excellent [OPTIONS]

    <PATH1> [<PATH2> ...] … $ # Read the help $ excellent --checks $ # Look at the list
  74. $ excellent app lib -o reports/excellent.html $ firefox reports/excellent.html $

    # Look at the report
  75. # app/mailers/notify.rb#38 # Assignment in condition. # ... def sender(sender_id)

    if sender = User.find(sender_id) # ...
  76. Code Smells: • Bad Practices • Actual Error • Could

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

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

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

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

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

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

    ... def parallel_diff_lines(project, commit, diff, file) # ...
  83. Cyclomatic Complexity = nbr of possible paths def hello_world puts

    "Hello world" end
  84. def hello_world puts "Hello world" end Cyclomatic Complexity = 1

  85. def hello(name) if name puts "Hello #{name}" end end CC

    = 2
  86. Cyclomatic Complexity = nbr of branches + 1

  87. def hello(name) return unless name puts "Hello #{name}" end CC

    = 1
  88. Cyclomatic Complexity = nbr of branches - nbr of exits

    + 2
  89. Complexity Refactoring: • app/mailers/emails/projects.rb • app/services/projects/update_service.rb • app/services/projects/create_service.rb

  90. Complexity Refactoring: • Extract Method • SRP, guard clauses •

    … Goal: Spread the Complexity
  91. # app/mailers/emails/projects.rb#19 # it has 5 parameters # ... def

    repository_push_email(project_id, recipient, author_id, branch, compare) # ...
  92. Parameters Refactoring: • SRP!

  93. Brakeman Security

  94. URI: https://github. com/presidentbeef/brakeman Purpose: Spot Security Vulnerabilities http://brakemanscanner. org/docs/warning_types/

  95. $ gem install brakeman $ brakeman --help Usage: brakeman [options]

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

    report $ brakeman . -o reports/brakeman.html $ firefox reports/brakeman.html $ # Look at the report
  97. # app/controllers/files_controller.rb # Unvalidated Redirect class FilesController < ApplicationController def

    download # ...
  98. # app/controllers/projects/ # team_members_controller.rb#22 # Unvalidated Redirect def create #

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

    vulnerability # (CVE-2014-3482). Upgrade to 4.1.3 # … gem "rails", "~> 4.1.0" # …
  100. Security Refactoring Basics: • No Secret • Not Directly Driven

    by User Data • Rails Version
  101. Rubocop Style

  102. URI: https://github.com/bbatsov/rubocop Purpose: Code Style and moar https://github.com/bbatsov/ruby- style-guide https://github.

    com/bbatsov/rubocop#cops
  103. $ gem install rubocop $ rubocop --help Usage: rubocop [options]

    [file1, file2, ...] … $ # Read the help
  104. $ rubocop app -f o -o reports/rubocop_summary.out $ # Look

    at the report
  105. 538 Style/StringLiterals 499 Style/LineLength 195 Style/Documentation ...

  106. $ rubocop app -f s -o reports/rubocop.out $ # Look

    at the report
  107. == 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. ...
  108. $ # A few edits later $ rubocop app/mailers/notify.rb -f

    s $ # Look at the report
  109. == 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.
  110. Style Refactoring Basics: • No Truth, Just Convention • Read

    by Humans not Machine • Do not Surprise you and others
  111. Rails Best Practices

  112. URI: https://github. com/railsbp/rails_best_practices Purpose: Rails Best Practices https://github. com/railsbp/rails_best_practices#im plementation

    http://rails-bestpractices.com/
  113. $ gem install rails_best_practices $ rails_best_practices --help Usage: rails_best_practices [options]

    path … $ # Read the help
  114. $ rails_best_practices -f html -- output-file reports/rails_best_practices.html $ firefox reports/rails_best_practices.html

    $ # Look at the reports
  115. $ # Uncheck All $ # 1 Always Add Db

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

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

    # db/migrate/ 20140416074002_add_index_on_iid.rb
  118. $ # 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
  119. $ # 3 Law of Demeter $ # app/mailers/emails/projects.rb#7

  120. $ # 3 Law of Demeter $ # app/mailers/emails/projects.rb#7 $

    # app/models/user_projects.rb#27
  121. $ # 4 Replace Instance Variable With Local Variable $

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

    # app/views/projects/tree/ _tree_item.html.haml#5 $ # app/helpers/tree_helper.rb#13
  123. 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"
  124. Yard / Inch Documentation

  125. URI: https://github.com/lsegal/yard Purpose: Find missing doc

  126. $ gem install yard $ yard stats --list-undoc > reports/yard.out

    $ # app/controllers/admin/ background_jobs_controller.rb
  127. 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
  128. URI: https://github.com/rrrene/inch Purpose: Improve Documentation http://trivelop.de/inch/

  129. $ gem install inch $ inch suggest --all app/ >

    reports/inch.out $ cat reports/inch.out
  130. Reek Code Smells

  131. URI: https://github. com/troessner/reek Purpose: Code Smells https://github. com/troessner/reek/wiki/Code-Smells

  132. $ gem install reek $ reek -S app/ > reports/reek.txt

    $ # app/helpers/commits_helper.rb
  133. $ # app/helpers/commits_helper.rb $ # Duplication Call $ # 99,

    99
  134. $ # app/helpers/commits_helper.rb $ # UtilityFunction & FeatureEnvy $ #

    98
  135. $ # app/helpers/commits_helper.rb $ # BooleanParameter,ControlParameter $ # 60

  136. $ # app/helpers/commits_helper.rb $ # Uncommunicative Variable Name $ #

    85
  137. $ # app/helpers/commits_helper.rb $ # Unused parameter $ # 18

  138. $ # app/helpers/commits_helper.rb $ # NilCheck $ # 18

  139. Driven Refactoring?

  140. It Drives ➔ Without deep knowledge ➔ Without a peer

    ➔ Enough feedback
  141. SDLC & Auto

  142. When do I run them?

  143. None
  144. How to Automate?

  145. ➔ rom-rb/devtools ➔ rake task ➔ as a service (PullReview,

    CodeClimate, HoundCI, etc.)
  146. # 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
  147. # 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
  148. How to start?

  149. ➔ "Agile", step by step ➔ 1st: 1 tool &

    locally ➔ 2nd: show off ➔ 3rd: CI ➔ 4th: reapeat
  150. Next: your project?

  151. ? https://github.com/ 8thcolor/eurucamp2014-htdsadr _toch toch