Tweet
Tweet

Slide 1

Slide 1 text

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

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

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 ➔ dead code

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

When you start, you build a kart.

Slide 43

Slide 43 text

No content

Slide 44

Slide 44 text

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

Slide 45

Slide 45 text

No content

Slide 46

Slide 46 text

Inertia is not the same

Slide 47

Slide 47 text

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

Slide 48

Slide 48 text

No content

Slide 49

Slide 49 text

No content

Slide 50

Slide 50 text

What's refactoring?

Slide 51

Slide 51 text

➔ Restructuring existing Without changing behavior ➔ Improves structural quality

Slide 52

Slide 52 text

The Toolbelt

Slide 53

Slide 53 text

Setup The project

Slide 54

Slide 54 text

GitLab Setup Instructions ➔ Big ➔ I don't know ➔ Open Source ➔ Rails app

Slide 55

Slide 55 text

# 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

Slide 56

Slide 56 text

$ mkdir reports

Slide 57

Slide 57 text

➔ Flay ➔ Excellent ➔ Brakeman ➔ Rubocop ➔ Rails Best Practices ➔ Yard / inch ➔ Reek

Slide 58

Slide 58 text

Flay Duplication

Slide 59

Slide 59 text

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

Slide 60

Slide 60 text

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

Slide 61

Slide 61 text

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

Slide 62

Slide 62 text

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

Slide 63

Slide 63 text

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

Slide 64

Slide 64 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 65

Slide 65 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 66

Slide 66 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 67

Slide 67 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 68

Slide 68 text

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

Slide 69

Slide 69 text

Dup Refactoring: ● app/mailers/emails/notes.rb ● app/controllers/{dashboard,groups} _controller.rb ● app/services/{issues,merge_requests} /update_service.rb

Slide 70

Slide 70 text

Dup Refactoring: ● Extract Method ● Pull-Up ● Strategy or Mixin

Slide 71

Slide 71 text

Excellent Code Smells

Slide 72

Slide 72 text

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

Slide 73

Slide 73 text

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

Slide 74

Slide 74 text

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

Slide 75

Slide 75 text

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

Slide 76

Slide 76 text

Code Smells: ● Bad Practices ● Actual Error ● Could be an error ● Could lead to an error

Slide 77

Slide 77 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 78

Slide 78 text

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

Slide 79

Slide 79 text

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

Slide 80

Slide 80 text

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

Slide 81

Slide 81 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 82

Slide 82 text

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

Slide 83

Slide 83 text

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

Slide 84

Slide 84 text

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

Slide 85

Slide 85 text

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

Slide 86

Slide 86 text

Cyclomatic Complexity = nbr of branches + 1

Slide 87

Slide 87 text

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

Slide 88

Slide 88 text

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

Slide 89

Slide 89 text

Complexity Refactoring: ● app/mailers/emails/projects.rb ● app/services/projects/update_service.rb ● app/services/projects/create_service.rb

Slide 90

Slide 90 text

Complexity Refactoring: ● Extract Method ● SRP, guard clauses ● … Goal: Spread the Complexity

Slide 91

Slide 91 text

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

Slide 92

Slide 92 text

Parameters Refactoring: ● SRP!

Slide 93

Slide 93 text

Brakeman Security

Slide 94

Slide 94 text

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

Slide 95

Slide 95 text

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

Slide 96

Slide 96 text

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

Slide 97

Slide 97 text

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

Slide 98

Slide 98 text

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

Slide 99

Slide 99 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 100

Slide 100 text

Security Refactoring Basics: ● No Secret ● Not Directly Driven by User Data ● Rails Version

Slide 101

Slide 101 text

Rubocop Style

Slide 102

Slide 102 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 103

Slide 103 text

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

Slide 104

Slide 104 text

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

Slide 105

Slide 105 text

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

Slide 106

Slide 106 text

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

Slide 107

Slide 107 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 108

Slide 108 text

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

Slide 109

Slide 109 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 110

Slide 110 text

Style Refactoring Basics: ● No Truth, Just Convention ● Read by Humans not Machine ● Do not Surprise you and others

Slide 111

Slide 111 text

Rails Best Practices

Slide 112

Slide 112 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 113

Slide 113 text

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

Slide 114

Slide 114 text

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

Slide 115

Slide 115 text

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

Slide 116

Slide 116 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 117

Slide 117 text

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

Slide 118

Slide 118 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 119

Slide 119 text

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

Slide 120

Slide 120 text

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

Slide 121

Slide 121 text

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

Slide 122

Slide 122 text

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

Slide 123

Slide 123 text

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"

Slide 124

Slide 124 text

Yard / Inch Documentation

Slide 125

Slide 125 text

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

Slide 126

Slide 126 text

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

Slide 127

Slide 127 text

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

Slide 128

Slide 128 text

URI: https://github.com/rrrene/inch Purpose: Improve Documentation http://trivelop.de/inch/

Slide 129

Slide 129 text

$ gem install inch $ inch suggest --all app/ > reports/inch.out $ cat reports/inch.out

Slide 130

Slide 130 text

Reek Code Smells

Slide 131

Slide 131 text

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

Slide 132

Slide 132 text

$ gem install reek $ reek -S app/ > reports/reek.txt $ # app/helpers/commits_helper.rb

Slide 133

Slide 133 text

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

Slide 134

Slide 134 text

$ # app/helpers/commits_helper.rb $ # UtilityFunction & FeatureEnvy $ # 98

Slide 135

Slide 135 text

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

Slide 136

Slide 136 text

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

Slide 137

Slide 137 text

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

Slide 138

Slide 138 text

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

Slide 139

Slide 139 text

Driven Refactoring?

Slide 140

Slide 140 text

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

Slide 141

Slide 141 text

SDLC & Auto

Slide 142

Slide 142 text

When do I run them?

Slide 143

Slide 143 text

No content

Slide 144

Slide 144 text

How to Automate?

Slide 145

Slide 145 text

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

Slide 146

Slide 146 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` `inch suggest --all app > reports/inch.out` end

Slide 147

Slide 147 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 'inch', require: false # gem 'rails_best_practices' already there end

Slide 148

Slide 148 text

How to start?

Slide 149

Slide 149 text

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

Slide 150

Slide 150 text

Next: your project?

Slide 151

Slide 151 text

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