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

Slide 1

Slide 1 text

How to do Static Analysis Driven Refactoring Christophe Philemotte, 2014-08-01, @eurucamp 2014

Slide 2

Slide 2 text

_toch toch Hi, I'm Christophe

Slide 3

Slide 3 text

Belgium

Slide 4

Slide 4 text

Belgium

Slide 5

Slide 5 text

Co-founder of

Slide 6

Slide 6 text

No content

Slide 7

Slide 7 text

Code = Garden

Slide 8

Slide 8 text

No content

Slide 9

Slide 9 text

How?

Slide 10

Slide 10 text

No content

Slide 11

Slide 11 text

→ Every Day → Equipped → Skilled

Slide 12

Slide 12 text

Outline

Slide 13

Slide 13 text

→ What & why? → Toolbelt → SDLC & Auto → Moar Exercises

Slide 14

Slide 14 text

Workshop Dynamics?

Slide 15

Slide 15 text

Please Interact!

Slide 16

Slide 16 text

Please Interact!!

Slide 17

Slide 17 text

Please Interact!!!

Slide 18

Slide 18 text

What & Why?

Slide 19

Slide 19 text

What's Static Analysis?

Slide 20

Slide 20 text

Analysis of your software without executing it

Slide 21

Slide 21 text

== Code Review

Slide 22

Slide 22 text

== Code Review but with a program

Slide 23

Slide 23 text

Code Read IR Analyze Results

Slide 24

Slide 24 text

# stupid.rb var = 1

Slide 25

Slide 25 text

$ 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<---------------

Slide 26

Slide 26 text

$ ruby -wc stupid.rb stupid.rb:1: warning: assigned but unused variable - test Syntax OK

Slide 27

Slide 27 text

$ # Get all Ruby warnings $ grep rb_warn parse.y | > sed -e "/# define/d" | > sed -e "s/^[^\"]*\"//" | > sed -e "s/\"[^\"]*$//"

Slide 28

Slide 28 text

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

Slide 29

Slide 29 text

What cannot be detected?

Slide 30

Slide 30 text

What needs to be executed

Slide 31

Slide 31 text

➔ cpu & mem profiling ➔ call graphs ➔ code coverage ➔ functionality / test ➔ race condition / deadlock

Slide 32

Slide 32 text

What can be detected?

Slide 33

Slide 33 text

➔ code smells ➔ flaws & defects ➔ vulnerabilities ➔ poor readability ➔ poor design ➔ poor documentation ➔ metrics // high risk of bugs

Slide 34

Slide 34 text

What are their benefits?

Slide 35

Slide 35 text

➔ it's a program ◆ indefatigable ◆ reliable ◆ reproducible ◆ focus and precise ◆ automation ➔ it drives refactoring & quality ➔ it saves time

Slide 36

Slide 36 text

What are their drawbacks?

Slide 37

Slide 37 text

➔ it's a program ◆ false positives ◆ cannot grasp business logic ◆ limited ➔ it needs setup & maintenance

Slide 38

Slide 38 text

Why should we use it?

Slide 39

Slide 39 text

➔ code review is done by a human ➔ tests are about a priori behaviors ➔ quality is important

Slide 40

Slide 40 text

Why quality is important?

Slide 41

Slide 41 text

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

Slide 42

Slide 42 text

➔ Worst case: better product for same effort ➔ Best case: better product for less effort ➔ 2 x → 100 x less expensive

Slide 43

Slide 43 text

No content

Slide 44

Slide 44 text

What's refactoring?

Slide 45

Slide 45 text

➔ Restructuring existing Without changing behavior ➔ Improves structural quality

Slide 46

Slide 46 text

The Toolbelt

Slide 47

Slide 47 text

Setup The project

Slide 48

Slide 48 text

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

Slide 49

Slide 49 text

$ git clone [email protected]: 8thcolor/eurucamp2014-htdsadr $ cd eurucamp2014-htdsadr $ bundle install --without mysql

Slide 50

Slide 50 text

$ cd config $ cp database.yml{.postgresql,} $ cp gitlab.yml{.example,} $ vi database.yml $ cd ..

Slide 51

Slide 51 text

$ 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

Slide 52

Slide 52 text

$ mkdir reports

Slide 53

Slide 53 text

➔ Flay ➔ Excellent ➔ Brakeman ➔ Rubocop ➔ Rails Best Practices ➔ Yard

Slide 54

Slide 54 text

Flay Duplication

Slide 55

Slide 55 text

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

Slide 56

Slide 56 text

$ gem install flay $ flay --help flay [options] files_or_dirs … $ # Read the help

Slide 57

Slide 57 text

$ flay . > reports/flay.out $ cat reports/flay.out $ # What do you notice?

Slide 58

Slide 58 text

$ flay -s . > flay_summary.out $ cat reports/flay_summary.out $ # Where do dups occur the most?

Slide 59

Slide 59 text

$ flay app > reports/flay_app.out $ cat reports/flay_app.out $ # What do you notice?

Slide 60

Slide 60 text

$ 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

Slide 61

Slide 61 text

# ... 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 # ...

Slide 62

Slide 62 text

# ... 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 # ...

Slide 63

Slide 63 text

# 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

Slide 64

Slide 64 text

$ flay app > reports/flay_app.out $ cat reports/flay_app.out $ # Dups are removed \o/

Slide 65

Slide 65 text

Excellent Code Smells

Slide 66

Slide 66 text

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

Slide 67

Slide 67 text

$ gem install excellent $ excellent --help Usage: excellent [OPTIONS] [ ...] … $ # Read the help $ excellent --checks $ # Look at the list

Slide 68

Slide 68 text

$ excellent app lib -o reports/excellent.html $ firefox reports/excellent.html $ # Look at the report

Slide 69

Slide 69 text

# app/mailers/notify.rb#38 # Assignment in condition. # ... def sender(sender_id) if sender = User.find(sender_id) # ...

Slide 70

Slide 70 text

# app/mailers/emails/projects.rb#19 # it has abc score of 17.03 # ... def repository_push_email(project_id, recipient, author_id, branch, compare) # ...

Slide 71

Slide 71 text

|ABC| = sqrt( A * A + B * B + C * C ) A = Assignment B = Branch C = Condition

Slide 72

Slide 72 text

# app/workers/post_receive.rb#7 # it has flog score of 32. # ... def perform(repo_path, oldrev, newrev, ref, identifier) # ...

Slide 73

Slide 73 text

Flog = ABC for Ruby C = Calls + weight, e.g. metaprogramming https://github.com/seattlerb/flog/blob/master/lib/flog.rb#L13-67

Slide 74

Slide 74 text

Good ABC/Flog Score? 0-10 Awesome 11-20 Good enough 21-40 Might need refactoring 41-60 Possible to justify 61+ Danger

Slide 75

Slide 75 text

# app/helpers/commits_helper.rb#107 # it has cyclomatic complexity of 18. # ... def parallel_diff_lines(project, commit, diff, file) # ...

Slide 76

Slide 76 text

Cyclomatic Complexity = nbr of possible paths def hello_world puts "Hello world" end

Slide 77

Slide 77 text

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

Slide 78

Slide 78 text

def hello(name) if name puts "Hello #{name}" end end CC = 2

Slide 79

Slide 79 text

Cyclomatic Complexity = nbr of branches + 1

Slide 80

Slide 80 text

def hello(name) return unless name puts "Hello #{name}" end CC = 1

Slide 81

Slide 81 text

Cyclomatic Complexity = nbr of branches - nbr of exits + 2

Slide 82

Slide 82 text

# app/mailers/emails/projects.rb#19 # it has 5 parameters # ... def repository_push_email(project_id, recipient, author_id, branch, compare) # ...

Slide 83

Slide 83 text

Brakeman Security

Slide 84

Slide 84 text

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

Slide 85

Slide 85 text

$ gem install brakeman $ brakeman --help Usage: brakeman [options] rails/root/path … $ # Read the help $ brakeman --checks $ # Look at the list

Slide 86

Slide 86 text

$ brakeman . -o reports/brakeman.out $ # Look at the report $ brakeman . -o reports/brakeman.html $ firefox reports/brakeman.html $ # Look at the report

Slide 87

Slide 87 text

# app/controllers/files_controller.rb # Unvalidated Redirect class FilesController < ApplicationController def download # ...

Slide 88

Slide 88 text

# app/controllers/projects/ # team_members_controller.rb#22 # Unvalidated Redirect def create # … if params[:redirect_to] redirect_to params[:redirect_to] # …

Slide 89

Slide 89 text

# Gemfile#11 # Rails 4.1.1 contains a SQL # injection vulnerability # (CVE-2014-3482). Upgrade to 4.1.3 # … gem "rails", "~> 4.1.0" # …

Slide 90

Slide 90 text

Rubocop Style

Slide 91

Slide 91 text

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

Slide 92

Slide 92 text

$ gem install rubocop $ rubocop --help Usage: rubocop [options] [file1, file2, ...] … $ # Read the help

Slide 93

Slide 93 text

$ rubocop app -f o -o reports/rubocop_summary.out $ # Look at the report

Slide 94

Slide 94 text

538 Style/StringLiterals 499 Style/LineLength 195 Style/Documentation ...

Slide 95

Slide 95 text

$ rubocop app -f s -o reports/rubocop.out $ # Look at the report

Slide 96

Slide 96 text

== 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. ...

Slide 97

Slide 97 text

$ # A few edits later $ rubocop app/mailers/notify.rb -f s $ # Look at the report

Slide 98

Slide 98 text

== 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.

Slide 99

Slide 99 text

Rails Best Practices

Slide 100

Slide 100 text

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/

Slide 101

Slide 101 text

$ gem install rails_best_practices $ rails_best_practices --help Usage: rails_best_practices [options] path … $ # Read the help

Slide 102

Slide 102 text

$ rails_best_practices -f html -- output-file reports/rails_best_practices.html $ firefox reports/rails_best_practices.html $ # Look at the reports

Slide 103

Slide 103 text

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

Slide 104

Slide 104 text

$ # Uncheck All $ # 1 Always Add Db Index $ # db/schema.rb#68 $ RAILS_ENV=test rails generate migration AddIndexToForkedProjectLinks $ RAILS_ENV=test rake db:migrate

Slide 105

Slide 105 text

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

Slide 106

Slide 106 text

$ # 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

Slide 107

Slide 107 text

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

Slide 108

Slide 108 text

$ # 3 Law of Demeter $ # app/mailers/emails/projects.rb#7 $ # app/models/user_projects.rb#27

Slide 109

Slide 109 text

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

Slide 110

Slide 110 text

$ # 4 Replace Instance Variable With Local Variable $ # app/views/projects/tree/ _tree_item.html.haml#5 $ # app/helpers/tree_helper.rb#13

Slide 111

Slide 111 text

Yard Documentation

Slide 112

Slide 112 text

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

Slide 113

Slide 113 text

$ gem install yard $ yard stats --list-undoc > reports/yard.out $ # app/controllers/admin/ background_jobs_controller.rb

Slide 114

Slide 114 text

Driven Refactoring?

Slide 115

Slide 115 text

It Drives ➔ Without deep knowledge ➔ Without a peer ➔ Enough feedback

Slide 116

Slide 116 text

SDLC & Auto

Slide 117

Slide 117 text

When do I run them?

Slide 118

Slide 118 text

No content

Slide 119

Slide 119 text

How to Automate?

Slide 120

Slide 120 text

➔ rom-rb/devtools ➔ rake task ➔ as a service (PullReview, CodeClimate, HoundCI, etc.)

Slide 121

Slide 121 text

# 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

Slide 122

Slide 122 text

# 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

Slide 123

Slide 123 text

How to start?

Slide 124

Slide 124 text

➔ "Agile", step by step ➔ 1st: 1 tool & locally ➔ 2nd: show off ➔ 3rd: CI ➔ 4th: reapeat