Пишу на руби уже больше 4х лет
Когда-то коммитил в Travis CI
много чего еще
Вижу много кода, который мог бы быть лучше
Поделиться опытом
Получить фидбек
...
Со временем изменения вносить все тяжелее и тяжелее
Любое изменение требует все больше и больше времени
class Nameserver < ActiveRecord::Base
def self.import
nameservers = fetch_nameservers
geoip = GeoIP.new(Rails.root.join('db', 'GeoLiteCity.dat'))
nameservers.each do |nameserver|
Rails.logger.info "Processing nameserver: #{nameserver[:ip]}"
nameserver = nameserver.slice(:country_id, :city, :state, :ip)
if valid_for_import?(nameserver)
fill_nameserver_with_additional_attributes(geoip, nameserver)
create!(nameserver)
end
end
...
...
def self.fill_nameserver_with_additional_attributes(nameserver)
nameserver[:city] = nameserver[:city].presence
city = geoip.city(nameserver[:ip])
nameserver[:latitude] = city.latitude
nameserver[:longitude] = city.longitude
end
def self.fetch_nameservers
open('http://public-dns.tk/nameservers.json') { |f|
JSON.parse(f.read).map(&:symbolize_keys)
}
end
...
...
def self.valid_for_import?(nameserver)
valid_country?(nameserver) &&
valid_state?(nameserver) &&
valid_ip_address?(nameserver) &&
exists?(:ip => nameserver[:ip]) &&
is_able_to_resolve?(nameserver)
end
private
# def self.valid_country?(nameserver)
# def self.valid_state?(nameserver)
# def self.valid_ip_address?(nameserver)
# def self.is_able_to_resolve?(nameserver)
...
...
# Еще 10 страниц кода в том же классе
# И этот код уже не связан с импортом
end
end
Литературы очень много
Low coupling
High cohesion
Small classes
Composition over inheritance
...
class Nameservers::Importer
def initialize(options)
@logger = options.fetch(:logger) {
Rails.logger
}
@repository = options.fetch(:repository) {
Nameservers::Nameserver
}
@validator = options.fetch(:validator) {
Nameservers::Validator.new(:repository => @repository)
}
@fetches_namespace_data = options.fetch(:fetches_namespace_data) {
Nameservers::DataLoader(:geoip =>
GeoIP.new(Rails.root.join('db', 'GeoLiteCity.dat')))
}
end
class Nameservers::Importer
def call(nameservers)
nameservers.each do |nameserver|
@logger.info "Processing nameserver: #{nameserver[:ip]}"
nameserver = nameserver.slice(:country_id, :city, :state, :ip)
if @validator.valid?(nameserver)
nameserver = @fetches_namespace_data.call(nameserver)
@repository.create!(nameserver)
end
end
end
end
Single responsibility principle
Open closed principle
Liskov substitution principle
Interface segregation principle
Dependency inversion principle
Создавайте больше классов.
При создании задумывайтесь, что может меняться.
Выделяйте зависимости (еще больше классов).
Изолируйте изменения.
class Invoice < ActiveRecord::Base
attr_accessible :title
def as_json(options = {})
super(methods: [:user_name], only: [:id, :date_range, :price])
end
def report(filename)
CSV.generate(filename)
# ...
end
def background_color
settings.background_color || 'rgba(0, 0, 255, 0.5)'
end
def schedule_post_upload_processing
InvoiceProcessor.delay(priority: 15).after_create(id)
end
end
class Invoice < ActiveRecord::Base # persistence
attr_accessible :title # fixed with strong params, do you agree?
def as_json(options = {}) # ActiveModel::Serializers, CustomSerializer
super(methods: [:user_name], only: [:id, :date_range, :price])
end
def report(filename) # InvoiceReportGenerator, ReportGenerator?
CSV.generate(filename)
# ...
end
def background_color # really? background color?
settings.background_color || 'rgba(0, 0, 255, 0.5)'
end
def schedule_post_upload_processing # definitely persistence
InvoiceProcessor.delay(priority: 15).after_create(id)
end
end
Идея - по возможности stateless классы подобного вида:
class Service
def initialize(*dependencies)
# @dependencies = dependencies
end
# methods that use dependencies
end
Тем, кто писал на Angular.js будет знакома концепция
class Nameservers::Importer
def initialize(logger, repository, geoip, validator)
end
end
class Nameservers::Validator
def initialize(logger, repository)
end
end
class Nameservers::EventLogger
def initialize(logger_file_path)
end
end
module Nameservers::Registry
def self.repository
Nameservers::Nameserver # < ActiveRecord::Base
end
def self.importer
@importer ||= Nameservers::Importer.new(:validator => validator,
:repository => repository,
:geoip => Geoip.new(root + '/db/geoipcitylite.dat'))
end
end
module Nameservers::Registry
def self.validator
Nameservers::Validator.new(:repository => repository)
end
def self.serializer
Oj
end
private
def self.root
File.expand_path('../../', __FILE__)
end
end
container.register :root_path, '/tmp'
container.register :repository, Nameserver
container.register :validator do
NameserverValidator.new(:repository => repository)
end
container.register :importer do |validator, repository, root_path|
NameserverImporter.new(:validator => validator,
:repository => repository,
:geoip => Geoip.new(root_path + '/db/geoipcitylite.dat'))
end
Можно использовать вот такие "контейнеры" как для всего приложения, так и для конкретного запроса.
Их даже можно обьединить.
При использовании такой схемы, можно сделать обертку над ActionController, которая будет автоматически вставлять аргументы:
class ImportController
def import_nameservers(params, importer, nameserver_data_source)
importer.call(nameserver_data_source.all(params[:data_source_url]))
end
end
Hint: poniard
Можно сделать проще:
class ApplicationController
def container
# setup and cache container
end
end
class ImportController
def import_nameservers
nameservers = container[:nameserver_data_source].all(params[:data_source_url])
container[:importer].call(nameservers)
end
end
class Nameserver < ActiveRecord::Base
def on(event, &callback)
(@events[event] ||= []) << callback
end
def trigger(event, *args)
@events[event].each do |callback|
callback.call(*args)
end
end
end
class Nameserver < ActiveRecord::Base
around_save :notify_on_save
private
def notify_on_save
if new_record?
# ...
end
yield
if connection_address_changed?
trigger(:connection_address_changed, new_connection_address, old_connection_address)
end
# ...
end
end
class NameserversController
def update
@nameserver = Nameserver.find(params[:ip])
@nameserver.on :become_available do
schedule_background_job_for(@nameserver)
end
@nameserver.on :connection_address_changed do |new_connection_address, old_connection_address|
notify_via_websockets(:create_connection, new_connection_address)
notify_via_websockets(:delete_connection, old_connection_address)
end
@nameserver.save
end
end
Вместо коллбеков можно использовать сам контроллер:
class Nameserver < ActiveRecord::Base
def on(event, observer)
(@observers[event] ||= []) << observer
end
def trigger(event, *args)
@observers[event].each do |observer|
if observer.respond_to?(event)
observer.send(event, self, *args)
end
end
end
end
class NameserversController
def update
@nameserver = Nameserver.find(params[:ip])
@nameserver.add_listener self
@nameserver.save
end
def connection_address_changed(nameserver, new_connection_address, old_connection_address)
notify_via_websockets(:create_connection, new_connection_address)
notify_via_websockets(:delete_connection, old_connection_address)
end
end
... или другие обьекты
class NameserversController
def update
@nameserver = Nameserver.find(params[:ip])
@nameserver.add_listener WebsocketNotifier.new(websocket_server_url)
@nameserver.save
end
end
class WebsocketNotifier
def initialize(websocket_server_url)
@websocket_server_url = websocket_server_url
end
def connection_address_changed(nameserver, new_connection_address, old_connection_address)
# NOTIFY
end
end
class EventSet
def on(event, observer)
@observers ||= {}
@observers[event] ||= []
@observers[event] << observer
end
def off(event, observer)
return unless @observers
return unless @observers[event]
@observers[event].delete(observer)
end
def trigger(event, *args)
if @observers && @observers[event]
@observers[event].each do |observer|
if observer.respond_to?(event)
observer.send(event, *args)
end
end
end
end
end
Идея - отделить реакцию на запрос от фреймворка
class CreatesUserFile # or UserFileCreator
def initialize(repository, uploader)
@repository = repository
@uploader = uploader
end
def call(params, request_body, listener)
attributes = extract_attributes(params)
attributes[:file_url] = @uploader.upload(request_body)
file = @repository.create(attributes)
listener.creation_succeeded(file)
rescue => e
listener.creation_failed(e)
end
end
Приняли аргументы - возвратите значение
Не изменяйте переданные аргументы
def fill_nameserver_attributes(nameserver)
city = $geoip.city nameserver[:ip].to_s
nameserver[:latitude] = city.latitude
nameserver[:longitude] = city.longitude
end
nameserver = {:ip => IPAddr.new('8.8.8.8') }
fill_nameserver_attributes(nameserver)
nameserver[:latitude]
=> 32.2
def fill_nameserver_attributes(nameserver)
city = $geoip.city nameserver[:ip].to_s
nameserver.merge :latitude => city.latitude,
:longitude => city.longitude
end
Если производительность вам в данном куске кода ОЧЕНЬ важна (что маловероятно):
def fill_nameserver_attributes(nameserver)
city = $geoip.city nameserver[:ip].to_s
nameserver[:latitude] = city.latitude
nameserver[:longitude] = city.longitude
nameserver
end
API должно показывать что клиент должен рассчитывать только на возвращаемое значение.
class JSONLoggerFormatter
def call(message)
message.to_json << "\n"
end
end
#to_json
class JSONLoggerFormatter
def call(message)
JSON.dump(message) << "\n"
end
end
class JSONLoggerFormatter
def initialize(json)
@json = json
end
def call(message)
@json.dump(message) << "\n"
end
end
class JSONLoggerFormatter
def initialize(json)
@json = json
end
def call(message)
@json.dump(message.to_value) << "\n"
end
end
class SerializedLoggerFormatter
def initialize(serializer)
@serializer = serializer
end
def call(message)
@serializer.dump(message.to_value) << "\n"
end
end