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/

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
7. Code = Garden
9. How?
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.
44. Eventually, you'll end with a Boïng.
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
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?
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