Eurucamp 2014 Workshop: How to do Static Analysis Driven Refactoring?

Eurucamp 2014 Workshop: How to do Static Analysis Driven Refactoring?

Workshop given at Eurucamp 2014.

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.

Setup Instructions: https://gist.github.com/toch/ac3ca5d7e565ee3dc375

Exercises: https://github.com/8thcolor/eurucamp2014-htdsadr

3f8fcddf7ab5d1bd90b0a0a9adfd6527?s=128

Christophe Philemotte

August 01, 2014
Tweet

Transcript

  1. How to do Static Analysis Driven Refactoring Christophe Philemotte, 2014-08-01,

    @eurucamp 2014
  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

    → Moar Exercises
  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
  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. ➔ Worst case: better product for same effort ➔ Best

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

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

  46. The Toolbelt

  47. Setup The project

  48. GitLab URI: https://github.com/ 8thcolor/eurucamp2014-htdsadr ➔ Big ➔ I don't know

    ➔ Open Source ➔ Rails app
  49. $ git clone git@github.com: 8thcolor/eurucamp2014-htdsadr $ cd eurucamp2014-htdsadr $ bundle

    install --without mysql
  50. $ cd config $ cp database.yml{.postgresql,} $ cp gitlab.yml{.example,} $

    vi database.yml $ cd ..
  51. $ 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
  52. $ mkdir reports

  53. ➔ Flay ➔ Excellent ➔ Brakeman ➔ Rubocop ➔ Rails

    Best Practices ➔ Yard
  54. Flay Duplication

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

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

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

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

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

    What do you notice?
  60. $ 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
  61. # ... 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 # ...
  62. # ... 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 # ...
  63. # 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
  64. $ flay app > reports/flay_app.out $ cat reports/flay_app.out $ #

    Dups are removed \o/
  65. Excellent Code Smells

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

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

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

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

    if sender = User.find(sender_id) # ...
  70. # app/mailers/emails/projects.rb#19 # it has abc score of 17.03 #

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

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

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

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

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

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

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

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

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

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

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

    + 2
  82. # app/mailers/emails/projects.rb#19 # it has 5 parameters # ... def

    repository_push_email(project_id, recipient, author_id, branch, compare) # ...
  83. Brakeman Security

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

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

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

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

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

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

    vulnerability # (CVE-2014-3482). Upgrade to 4.1.3 # … gem "rails", "~> 4.1.0" # …
  90. Rubocop Style

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

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

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

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

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

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

    s $ # Look at the report
  98. == 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.
  99. Rails Best Practices

  100. 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/
  101. $ gem install rails_best_practices $ rails_best_practices --help Usage: rails_best_practices [options]

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

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

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

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

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

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

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

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

    # app/views/projects/tree/ _tree_item.html.haml#5 $ # app/helpers/tree_helper.rb#13
  111. Yard Documentation

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

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

    $ # app/controllers/admin/ background_jobs_controller.rb
  114. Driven Refactoring?

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

    ➔ Enough feedback
  116. SDLC & Auto

  117. When do I run them?

  118. None
  119. How to Automate?

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

    CodeClimate, HoundCI, etc.)
  121. # 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` end
  122. # 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 'rails_best_practices' already there end
  123. How to start?

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

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

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