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

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
7. Code = Garden
9. How?
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
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?
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