the worst ruby codes i’ve seen in my life - rubykaigi 2015

The worst Ruby codes I’ve seen in my life

RubyKaigi 2015

@Prodis

Fernando Hamasaki de Amorim @Prodis

• Developing web applications for 15 years

• Writing Ruby code since 2009

• Working at Locaweb, the biggest hosting company in Brazil

• Playing basketball at the free time

WOP Workaround Oriented Programming

WOPWorkaround Oriented Programming is an advance

technique of software development that uses of any

kind of workaround, patch e all of evil a code can have.

WOP is based on code duplication, redundant flows,

unnecessary tasks and wheels reinvention.

"The names have been changed to protect the innocent."

A first example of WOP: masking credit card

numbers

describe '#mask_credit_card' do let(:number) { '5464193830403276' }

it 'returns masked credit card number' do masked = mask_credit_card(number) expect(masked).to eq '************3276' end end

def mask_credit_card(number) limit = number.length - 4 “#{'*' * limit}#{number[limit..-1]}” end

def mask_credit_card_wop(number) (number.length - 4).times do |i| number[i] = '*' end

number end

describe '#mask_credit_card_wop' do let(:number) { '5464193830403276' }

it 'returns masked credit card number' do masked = mask_credit_card_wop(number)

expect(masked).to eq '************3276' end

it 'does not change number variable' do mask_credit_card_wop(number) expect(number).to eq '5464193830403276' end end

#mask_credit_card_wop returns masked credit card number does not change number variable (FAILED - 1)

Failures:

1) #mask_credit_card_wop does not change number variable Failure/Error: expect(number).to eq '5464193830403276'

expected: "5464193830403276" got: "************3276"

(compared using ==) # ./spec/mask_credit_card/mask_credit_card_spec.rb:23:in `block (2 levels) in <top (required)>'

Finished in 0.0202 seconds (files took 0.17324 seconds to load) 2 examples, 1 failure

def mask_credit_card_wop(number) (number.length - 4).times do |i| number[i] = '*' end

number end

Unclear flow

class Support::DomainsController < Support::BaseController def create site = Site.find_by(name: params[:domain][:site])

if site.blank? flash[:alert] = I18n.t('support.domains.errors.without_site') redirect_to new_support_domain_path return else domain = site.domains.build(address: params[:domain][:address]) domain.support_create = true

if domain.save flash[:success] = I18n.t('support.domains.success') redirect_to support_domains_path return else flash[:alert] = I18n.t('support.domains.errors.invalid') redirect_to new_support_domain_path return end end end end

How to fix it?

unless site flash[:alert] = I18n.t('support.domains.errors.without_site') redirect_to new_support_domain_path return end

domain = site.domains.build(address: params[:domain][:address]) domain.support_create = true

unless domain.save flash[:alert] = I18n.t('support.domains.errors.invalid') redirect_to new_support_domain_path return end

flash[:success] = I18n.t('support.domains.success') redirect_to support_domains_path end end

unless site flash[:alert] = I18n.t('support.domains.errors.without_site') redirect_to new_support_domain_path return end

domain = site.domains.build(address: params[:domain][:address]) domain.support_create = true

unless domain.save flash[:alert] = I18n.t('support.domains.errors.invalid') redirect_to new_support_domain_path return end

flash[:success] = I18n.t('support.domains.success') redirect_to support_domains_path end end

No Ruby way

class PaymentGatewayWOP def initialize(options = {}) raise ArgumentError if options[:email].to_s.strip.empty? raise ArgumentError if options[:token].to_s.strip.empty?

@options = options end

def email @options[:email] end

def token @options[:token] end

def identification @options[:identification] end

def billing_type @options[:billing_type] end

def billing_status @options[:billing_status] end

def message @options[:message] end

def exists? @options[:message] =~ /Account found/ end

def is_active? @options[:billing_status] == 'active' end

def is_seller? @options[:billing_type] == 'seller' || @options[:billing_type] == 'company' end

# other methods omitted end

class PaymentGateway attr_reader :email, :token

def initialize(options = {}) @email = options.fetch(:email) @token = options.fetch(:token) @options = options end

def identification options[:identification] end

def billing_type options[:billing_type] end

# other public methods omitted

private

attr_reader :options

def identification options[:identification] end

def billing_type options[:billing_type] end

private

[:identification,:billing_type, :billing_status, :message].each do |method| define_method(method) do options[method] end end

private

# constructor omitted

[:identification, :billing_type, :billing_status, :message].each do |method| define_method(method) do options[method] end end

def exists? message =~ /Account found/ end

def is_active? billing_status == 'active' end

def is_seller? billing_type == 'seller' || billing_type == 'company' end

private

# other methods omitted

def active? billing_status == 'active' end

def seller? billing_type == 'seller' || billing_type == 'company' end

private

def billing_status @options[:billing_status] end

def message @options[:message] end

def exists? @options[:message] =~ /Account found/ end

def is_active? @options[:billing_status] == 'active' end

def is_seller? @options[:billing_type] == 'seller' || @options[:billing_type] == 'company' end

[:identification, :billing_type, :billing_status, :message].each do |method| define_method(method) do options[method] end end

def active? billing_status == 'active' end

def seller? billing_type == 'seller' || billing_type == 'company' end

private

# other methods end

Naming issues

class ImageWidgetImporter < WidgetImporter def import(img_element, row_number, position) return if img_element.blank? || img_element['src'].blank? create_image_widget(img_element, row_number, position) end

def import! @page.widgets.where(kind: 'text').each do |widget| content = Nokogiri::HTML(widget.content, nil, 'UTF-8') next unless has_internal_image?(content)

images = content.css('img').select do |image| internal_image?(image) end

images.each { |image| download_and_change_image_src(image) } widget.update_attribute(:content, content.inner_html) end end

private

def kind 'image' end

def create_image_widget(img_element, row_number, position) widget = create(row_number: row_number, position: position, remote_image_url: img_element['src']) source = (AppConfig.assets_host + widget.image.url) widget.content = @template_adapter.render_widget_content('image', alt: '', src: source) widget.save!

widget end

# Create widget_image to Text Widget def create_widget_image(url) widget_image = WidgetImage.new remote_image_url: url widget_image.site_id = @page.site.id widget_image.save!

widget_image end

private

def import_from_text_widget @page.widgets.where(kind: 'text').each do |widget| content = Nokogiri::HTML(widget.content, nil, 'UTF-8') next unless has_internal_image?(content)

private

class ImageWidgetImporter < WidgetImporter # other public methods omitted

private

widget end

# Create image widget to text widget def create_widget_image(url) widget_image = WidgetImage.new remote_image_url: url widget_image.site_id = @page.site.id widget_image.save!

widget_image end

private

widget end

# Create image widget to text widget def create_widget_image(url) widget_image = WidgetImage.new remote_image_url: url widget_image.site_id = @page.site.id widget_image.save!

widget_image end

private

widget end

def create_image_widget_to_text_widget(url) widget_image = WidgetImage.new remote_image_url: url widget_image.site_id = @page.site.id widget_image.save!

widget_image end

OOP Inheritance for the purpose

of code reuse

class Installation::FromFeed < Installation::FromBase def install(args) # implementation omitted end end

class Installation::FromHosting < Installation::FromBase def install(args) # implementation omitted end end

class Installation::FromMigration < Installation::FromBase def install(args) # implementation omitted end end

class Installation::FromBase include Rails::LabeledLog::Logging

attr_writer :customers_api, :installer, :mailer

def install(args) raise NotImplementedError end

def customers_api @customers_api ||= CustomersApi.new end

def installer @installer ||= Installation::Installer.new end

def mailer @mailer ||= Installation::Mailer.new end end

How to fix it?

module Installation::Infra include Rails::LabeledLog::Logging

attr_writer :customers_api, :installer, :mailer

def customers_api @customers_api ||= CustomersApi.new end

def installer @installer ||= Provisioner::Installation::Installer.new end

def mailer @mailer ||= Provisioner::Installation::Mailer.new end end

class Installation::FromFeed include Installation::Infra

def install(args) # implementation omitted end end

class Installation::FromHosting include Installation::Infra

class Installation::FromMigration include Installation::Infra

OOP Inheritance mistake

DNS A quick introduction

class WsDns attr_reader :host, :user, :timeout

def initialize(options) @host = options[:host] @user = options[:user] @timeout = options.fetch(:timeout, 5) end

def create_entry(options) # implementation omitted end

def delete_entry(options) # implementation omitted end

def get_entry(options) # implementation omitted end

def has_entry?(options) # implementation omitted end

# other methods to DNS zone end

class CnameWsDns attr_reader :ws_dns, :zone, :content

def initialize(options) @ws_dns = WsDns.new(options) @zone = options[:zone] @content = options.fetch(:content, zone) end

def create_entry(subdomain) ws_dns.create_entry(type: type, content: content, name: subdomain, zone: zone) end

def delete_entry(subdomain) ws_dns.delete_entry(type: type, content: content, name: subdomain, zone: zone) end

def has_entry?(subdomain) ws_dns.has_entry?(type: type, name: subdomain, zone: zone) end

protected

def type 'CNAME' end end

class AWsDns < CnameWsDns protected

def type 'A' end end

class TxtWsDns < CnameWsDns protected

def type 'TXT' end end

How to fix it?

OOP Parent knowing your children

class TransactionResponseParser attr_reader :xml

def initialize(xml) @xml = xml end

def parse # omitted implementation end

private

# specific transaction methods omitted end

class ResponseParser attr_reader :xml

def parse # omitted implementation end

# omitted protected methods end

class TransactionResponseParser < ResponseParser

private

# specific transaction methods omitted end

class AccountResponseParser < ResponseParser

private

# specific account methods omitted end

class ResponseParser def self.transaction?(xml) xml.include?('<transaction>') end

def self.get_parser(xml) ResponseParser.transaction?(xml) ? TransactionResponseParser.new(xml) : AccountResponseParser.new(xml) end

def parse # omitted implementation

end end

How to fix it?

module ResponseParserFactory def self.build(xml) if xml.include?('<transaction>') TransactionResponseParser.new(xml) else AccountResponseParser.new(xml) end end end

The worst class

class DomainChecker extend Memoist

DOMAIN_REGEXP = /^[a-z0-9]+(-[a-z0-9]+)*(\.[a-z0-9]+(-[a-z0-9]+)*)+$/

attr_accessor :domain

def initialize(args = {}) @domain = args[:domain] end

def check_new check_existing end

def status if dns_adapter.ns_locaweb? a_entry_locaweb = dns_adapter.a_entry_locaweb if a_entry_locaweb == AppConfig.ip_lvs_criador_de_sites return :ok elsif a_entry_locaweb == false return :unavailable else return :already_using end end

if domain_checker_result["error"] == "generic" return :generic_error end

if domain_checker_result["error"] == "unsupported_tld" return :unsupported_tld end

if domain_checker_result["available"] return :register end

if dns_adapter.a_value == AppConfig.ip_lvs_criador_de_sites return :ok else return :config_dns end end memoize :status

def available_domain_by_user(user) if domain.blank? return {valid: false, notice: :invalid, message: :blank} end

if !domain.match(DOMAIN_REGEXP) return {valid: false, notice: :invalid, message: :invalid} end

if forbidden_domain? return {valid: false, notice: :invalid, message: :forbidden_domain} end

if Domain.where(address: domain).count > 0 current_domain = Domain.where(address: domain).first if (current_domain.site.account.users.include?(user) rescue false) return {valid: false, notice: :invalid, message: :already_using} else return {valid: false, notice: :invalid, message: :already_exists} end end

if !domain_checker_result["valid"] && domain_checker_result["error"] != "unsupported_tld" return {valid: false, notice: :invalid, message: :invalid} end

if domain_checker_result["error"] == "unsupported_tld" return {valid: true, notice: :unsupported_tld} end

if domain_checker_result["available"] return {valid: true, notice: :register} end

if domain_checker_result["customer_login"].blank? return {valid: true, notice: :config_dns} end

if domain_checker_result["customer_login"].downcase == user.username.downcase Rails.logger.info "user owner domain"

if dns_adapter.a_entry_locaweb? if dns_adapter.a_entry_locaweb == AppConfig.ip_lvs_criador_de_sites_old return {valid: true, notice: :old_csit} else return {valid: true, notice: :already_using} end else Rails.logger.info "Without entry A" return {valid: true, notice: :owner_domain} end else Rails.logger.info "user does not owner domain" return {valid: false, notice: :not_owner} end end

def details { entry_a: dns_adapter.a_value, entry_ns: dns_adapter.ns_value, entry_cname: dns_adapter.cname_value }.merge(domain_checker_result) end

def check_existing return external_check if external_check["error"] == "generic" return external_check if external_check["error"] == "invalid_domain" return external_check if external_check["error"] == "unsupported_tld" return external_check if external_check["available"] return external_check if internal_check["available"] internal_check end

private

def dns_adapter DnsAdapter.new(domain: CGI.escape(domain)) end memoize :dns_adapter

def domain_checker_result domain_checker = DomainChecker.new(domain: CGI.escape(domain)) domain_checker_result = domain_checker.check_new end memoize :domain_checker_result

def get_token WsAuthentication.new(AppConfig.wsauthentication.url).authenticate(AppConfig.wsauthentication.user, AppConfig.wsauthentication.pass) end memoize :get_token

def external_check url = "#{AppConfig.registro_service_url}/domain_availability/#{domain}/external_check" begin JSON(http_get(url)) rescue RestClient::NotImplemented return { "valid" => false, "available" => false, "error" => 'unsupported_tld' } rescue RestClient::InternalServerError => exception Rails.logger.error "[ERROR] GET #{url}: #{exception.message}\n" \ "Response: #{exception.http_body}" return { "valid" => false, "available" => false, "error" => 'generic' } rescue => exception Rails.logger.error exception.print raise exception end end memoize :external_check

def internal_check url = "#{AppConfig.registro_service_url}/domain_availability/#{domain}/internal_check" JSON(http_get(url)) end memoize :internal_check

def forbidden_domain? uri = "#{AppConfig.registro_domain_url}/domain/#{domain}/check"

begin response = JSON(CasSaas::CasRestClient.new.get(uri))

!response["valid"] rescue => e Rails.logger.info e.message true end end memoize :forbidden_domain?

def http_get(url, headers = {}) Rails.logger.info "chamando GET #{url}, headers: #{headers}" response = RestClient.get url, headers Rails.logger.info "response #{response}" response end end

Business scenario for DomainChecker class

class DomainChecker # ...

def check_new # omitted implementation end

def status # omitted implementation end memoize :status

def available_domain_by_user(user) # omitted implementation end

def details # omitted implementation end

def check_existing # omitted implementation end

# ... end

def check_existing return external_check if external_check["error"] == "generic" return external_check if external_check["error"] == "invalid_domain" return external_check if external_check["error"] == "unsupported_tld" return external_check if external_check["available"] return external_check if internal_check["available"] internal_check

# ... end

def get_token WsAuthentication.new(AppConfig.wsauthentication.url).authenticate(AppConfig.wsauthentication.user, AppConfig.wsauthentication.pass)

end memoize :get_token

def forbidden_domain? uri = "#{AppConfig.registro_domain_url}/domain/#{domain}/check"

begin response = JSON(CasSaas::CasRestClient.new.get(uri)) !response["valid"] rescue => e Rails.logger.info e.message true end end memoize :forbidden_domain?

# ... end

def available_domain_by_user(user) # …

# … end

private

# ... end

private

# ... end

def http_get(url, headers = {}) Rails.logger.info "chamando GET #{url}, headers: #{headers}" response = RestClient.get url, headers Rails.logger.info "response #{response}" response end end

private

# ... end

private

# ... end

def details { entry_a: dns_adapter.a_value, entry_ns: dns_adapter.ns_value, entry_cname: dns_adapter.cname_value }.merge(domain_checker_result) end

private

#... end

class DomainChecker # ... private

def get_token WsAuthentication.new(AppConfig.wsauthentication.url).authenticate(AppConfig.wsauthentication.user, AppConfig.wsauthentication.pass)

end memoize :get_token end

DomainChecker class problems

Long class

Constructor with hash parameter, but only use

one key of the hash.

A method implementation that only call a private

method without parameters

Memoize (hell) dependency: memoized methods used

like variables.

A lot ifs: ifs with ifs with else with if with else

(it’s hard until to explain)

Code hard to understand

(internal x external checks)

An unused private method

Instance method creates another instance of itself

• Many responsibilities:

✴ Validate domain format

✴ Validate domain logic

✴ Format return to use in view

✴ Do HTTP requests

✴ Parse HTTP responses

DomainChecker class problems introduces new patterns and principles

Write-only codeOnce write no one can read

DomainChecker class introducesnew patterns and principles

Close Closed PrincipleClosed for modification,

more closed for extension.

Inception PatternWhere an instance of a class creates

a new instance of itself and aggregates the new instance state to your state

DomainChecker class probably is the worst Ruby class

I've seen in my life

How did we fix it?

Why is WOP applied?

Lack of knowledge

Causes of WOP

Immaturity in software development

Causes of WOP

No collaborative environment

Causes of WOP

No coaching

Causes of WOP

Tight deadlines

Causes of WOP

Why simplify if you can complicate?

Causes of WOP

To use "cool things" because they are cool, even

if they are not a solution.

Causes of WOP

Some mysteries of the human mind

(that we can't explain)

Causes of WOP

How to avoid WOP?

Read a lot

How to avoid WOP?

But do not learn only Ruby

How to avoid WOP?

Use code review

How to avoid WOP?

Read code from others programmers

How to avoid WOP?

Write code yourself can read in the future

How to avoid WOP?

Participate of open source projects: contributing, discussing, reading.

How to avoid WOP?

Do coaching of less experienced developers (teaching is a good way to learn too)

How to avoid WOP?

Do not write code for you: write it to the application,

to the team.

How to avoid WOP?

Exchange experiences, ask.

How to avoid WOP?

Use pair programming (not 100% IMHO)

How to avoid WOP?

Learn from your mistakes and others

How to avoid WOP?

Face bad code as an opportunity to get better

How to avoid WOP?

Do not face bad code with complaints or

making fun of the authors

How to avoid WOP?

This funny code causes waste of time, resources

and money

How to avoid WOP?

Instead, show the authors of bad code the right way

How to avoid WOP?

Show them the Ruby way

How to avoid WOP?

Thank you! I hope you enjoyed

@Prodis

RubyKaigi 2015

@Prodis

the worst ruby codes i’ve seen in my life - rubykaigi 2015

Software

coming events~ valley may newsletter 2019.pdf · best book...

present perfect 1ni 2012-2013. read the sentences i’ve...

worst floodingindecadesswampsthailand

rails girls: not only for girls - rubykaigi 2014

“this is the worst virus i’ve ever met in my long...

unit 4 dvd worksheet 1...unit 4 dvd worksheet 2 a d c e b...

worst presentationeverstandalone

why you should attend rubykaigi 2009

blacksmith instituteâ€™s world worst polluted world´s...

“this is the worst virus i’ve ever met in my long...

i’ve done, i’ve done, i’ve done, i’ve done my work...

rubykaigi 2010 babushka lightning talk

worst witch, the - the worst witch

avoiding the worst-case scenarios avoiding the worst-case...

a part of rubykaigi ecosystem

the worst of the worst - foreign policy jun 2010

10 secrets to an irresistible resume - new direction...

worst president

ruby m17n rubykaigi 08 rubykaigi 08 martin j. d ü rst

rubykaigi 2011 limited red talk