Code Quality in your organization

Tomasz Pajor
github: @nijikon
twitter: @tomaszpajor
e-mail: me@tomaszpajor.com

A bit about me

  • 10 years in IT
  • 5 years with Ruby and Rails
  • I <3 ruby
  • all in for open-source software
  • quality and automation are very important
  • i don't have a blog
  • i do consulting

class String;def[]*a;$*<<a;b;end;end;
_=0;z="C=Fiber;s=$*;a=*0..8;l=C.new{e
xit},*a.product(a).select{|r,c|s[r][c
]==0}."[1,9,_, _,_,8, _,_,5]+"map{|r,
c|C.ne"[_,_,2, _,5,_, _,8,9]+"w{o=s[r
][c];l"[8,_,6, 7,4,_, _,_,_]+"oop{(1.
.9).map{|n|C.yield(s[r][c]=n)if a.non
e?{|k|"[_,_,_, _,_,4, _,9,2]+"s[r][k]
==n||s"[_,2,3, _,7,_, 8,1,_]+"[k][c]=
=n||s["[5,6,_, 8,_,_, _,_,_]+"r-r%3+k
%3][c-c%3+k/3]==n}};s[r][c]=o;C.yield
}}},C."[_,_,_, _,2,7, 9,_,3]+"new{loo
p{puts"[9,3,_, _,8,_, 1,_,_]+" s.map{
|r|r*'"[2,_,_, 5,_,_, _,4,8]+" '}<<''
;C.yield}};c=l[i=1];loop{c=l[i+=c.res
ume ? 1:-1]}";eval z.tr ?\n,''

github.com/tric/trick2015

Please notify me if...

  • I speak 2 fast
  • I should repeat something
  • I should explain something better
  • You have any questions
  • Thanks Maciej Mensfeld for one more page in my slides ;)

Let's do this

Actually before we even start

Code quality is subjective, it requires human judgment

I think that we've all been there

Back to basics

Having cohesive coding style is very important

  • easier collaboration
  • reduces cost
  • everyone is happy :)

Clean code is easier to

  • understand
  • maintain
  • extend
  • plug in to other projects

Reduced clarity

Have you ever tried to find out how something works in ActiveRecord?

What if you actually need to fix something in ActiveRecord?

Let's get serious for a second

Reduced clarity

Long methods

Lots of methods in a single file


125 ./connection_adapters/abstract_mysql_adapter.rb
123 ./reflection.rb
117 ./migration.rb
86 ./relation/query_methods.rb
73 ./connection_adapters/postgresql_adapter.rb
72 ./connection_adapters/abstract_adapter.rb
68 ./connection_adapters/abstract/connection_pool.rb
66 ./connection_adapters/sqlite3_adapter.rb
63 ./connection_adapters/abstract/schema_statements.rb
62 ./fixtures.rb
58 ./connection_adapters/abstract/schema_definitions.rb
56 ./associations/collection_association.rb
52 ./connection_adapters/postgresql/schema_statements.rb
51 ./relation.rb
42 ./associations/collection_proxy.rb
41 ./core.rb
40 ./attribute_methods.rb
37 ./persistence.rb
37 ./associations.rb
36 ./relation/finder_methods.rb
35 ./transactions.rb
34 ./connection_adapters/mysql_adapter.rb
22 ./autosave_association.rb
15 ./nested_attributes.rb

Duplication

Single responsibility principle

Whole article is available at John Bohn blog

http://jjbohn.info/blog/2014/07/28/single-responsibility-principle-a-solid-week/


class Reporter
  def send_report
    users = User.where("last_logged_in_at >= ?", 1.week.ago)

    users.each do |user|
      message = "Id: #{user.id}\n"
      message += "Username: #{user.username}\n"
      message += "Last Login: #{user.last_logged_in_at.strftime("%D")}\n"
      message += "\n"
    end

    Mail.deliver do
      from "jjbohn@gmail.com"
      to "jill@example.com"
      subject "Your report"
      body message
    end
  end
end


class Reporter
  def send_report
    message = generate_message(users)

    deliver_report(message)
  end

  private

  def users
  end

  def generate_message(users)
    message = ""
    users.each do |user|
      message += "Id: #{user.id}\n"
      message += "Username: #{user.username}\n"
      message += "Last Login: #{user.last_logged_in_at.strftime("%D")}\n"
      message += "\n"
    end

    message
  end

  def deliver_report(message)
    Mail.deliver do
      from "jjbohn@gmail.com"
      to "jill@example.com"
      subject "Your report"
      body message
    end
  end
end


class ReportDataCsvCompiler
  attr_accessor :data
  def initialize(data)
    self.data = Array(data)
  end

  def format
    heading << body
  end

  private

  def heading
    data.first.keys.join(",") << "\n"
  end

  def body
    data.each_with_object("") do |str, row|
      str << row.values.join(",") << "\n"
    end
  end
end

class User
  scope :logged_in_this_week, ->{ where("last_logged_in_at >= ?", 1.week.ago) }
end

class UserWeeklyReport
  def self.name
    "Weekly User Report"
  end

  def self.formatter
    ReportDataCsvCompiler
  end

  def self.data
    UserWeeklyReportData
  end

  def to_file
    File.write("/tmp/my_report")
  end
end

class ReportMailer
  attr_accessor :report, :recipient

  def intiialize(report:, recipient:)
    self.report = report
    self.recipient = recipient
  end

  def deliver!
    mail = Mail.new do
      from "jjbohn@gmail.com"
      to recipient
      subject report.name
    end

    mail.add_file(report.to_file)
    mail.deliver!
  end
end

class UserWeeklyReportData
  def to_data
    User.logged_in_this_week.map do |user|
      {
        id: user.id,
        username: user.username,
        last_logged_in_at: user.last_logged_in_at.strftime("%D"),
      }
    end
  end
end

# Then in you client code
ReportMailer.new(UserWeeklyReport, "jill@example.com")

Over-engineering

  • one of the biggest issues right now
  • don't guess the future, operate on facts in present
  • don't make things that are sufficiently easy more complicated

Automation

There are tools

  • rubocop
  • flay
  • flog
  • reek
  • list goes on ...

Why should we use them?

People are expensive, CPU time is cheap ;)

Nobody will argue with a tasks over using " or '

In the long run using this kind of tools makes you a better developer

You will write clean and portable code

It will be easier to migrate or upgrade ruby / rails / gems

It's awesome when you run your code through codeclimate for the first time and see

How should I start using those tools?

We have something for you all

polishgeeks-dev-tools

github.com/polishgeeks/polishgeeks-dev-tools

polishgeeks-dev-tools


group :development, :test do
  gem 'polishgeeks-dev-tools'
end


# config/initializers/polishgeeks_dev_tools.rb
PolishGeeks::DevTools.setup

You are ready to go!


balrog:brief-api n$ be rake polishgeeks:dev-tools:check
RequiredFiles                                      OK
BundlerAudit                                       OK
Expires in                                         OK
Brakeman (9 con, 23 mod, 4 temp)                   OK
Rubocop (311 files, 0 offenses)                    OK
RubocopRspec (311 files, 0 offenses)               OK
Final blank line: 370 files checked                OK
Empty methods: 297 files checked                   OK
HamlLint                                           OK
Allowed Extensions                                 OK
Yaml parser                                        OK
Rspec files names: 50 files checked                OK
Tasks files names: 1 files checked                 OK
Rspec (427 ex, 0 fa, 0 pe)                         OK
SimpleCov covered 86.03%, required 85.0%           OK
ExamplesComparator                                 OK
Rubycritic                                         GENERATED

If you are not ready for some plugins, you can turn them off


PolishGeeks::DevTools.setup do |config|
  config.yard = false
  config.rspec_files_structure = false
  config.simplecov_threshold = 85
end

Current status

  • brakeman, bundler_audit, haml_lint, rubocop, rubocop_rspec, rubycritic, simplecov, yard
  • couple of own extensions
  • ideas on github

Sadly

You need ruby >= 2.1, but you should not use older anyway ;)

WANT TO CONTRIBUTE?

github.com/polishgeeks/polishgeeks-dev-tools

  • The more people star it, the more people use it
  • The more people use it, the more people star it
  • We are waiting for your ideas and suggestions

THE END - Q & A

github.com/polishgeeks/polishgeeks-dev-tools