Or: How to avoid and refactor spaghetti code
Please note that I tried to keep the examples small. The effects of the methods in this card are of course much more significant with real / more complex code.
What are the benefits of more modular code?
Code is written once but read often (by your future self and other developers who have to understand it in order to make changes for example). With more modular code you reduce the scope of what has to be understood in order to change something. Also, naming things gives you the opportunity to convey meaning which can be especially helpful for understanding.
Functions
When starting from scratch
Using code that does net yet exist
Sometimes it can be easier to find out the best API of a class by using that code first from the place that will be using it.
This will give you a better practical oriented viewpoint which exact methods will be required to make the usage of this class as easy and loosely coupled as possible.
- Write the view first for the required model
- Write the controller first for the required model
- Write the tests first for the code
bad:
While doing this you can for example see easier when you might be violating the
Law of Demeter
Show archive.org snapshot
.ul
@report.items.each do |item|
.li item.result.attribute
good:
.ul
@report.items.each do |item|
.li item.result_attribute
Write an outline first
When writing a new function, start with writing down what it has to do as functions - kind of the table of contents:
good:
def create_report
data = gather_data
warn_about_missing_data(data)
cleaned_data = clean_data(data)
generate_report(cleaned_data)
end
Now try to implement each new function from your list. Do this again by thinking about what the function has to do and writing an outline. Repeat until you have to actually implement something because you can no longer write outlines in a meaningful way.
However, if 90% of your functions only have one line of code you may have overdone it.
When refactoring code
This could be code from someone else or code you've just written.
Write a function instead of variable assignment
bad:
def my_function(employees)
contact = employees['product owner'] || employees['lead developer'] || employees['trainee']
do_something(contact)
# ...
end
better:
def my_function(employees)
contact = find_contact(employees)
do_something(contact)
# ...
end
private
def find_contact(employees)
employees['product owner'] || employees['lead developer'] || employees['trainee']
end
Write a function instead of adding a comment over a block
This also works well if you could think a comment over several lines of code.
bad:
def my_function
# prepare report data
customer_data = read(:customer_data, {})
customer_recommendations = read(:substituted_customer_recommendations, '')
report_data = customer_data.merge({customer_recommendations: customer_recommendations})
json_data = { report_data: report_data }
# write report data
filename = 'report_data.json'
result = Result.new(
file: FileIO.new(json_data.to_json, filename)
)
result.save!
end
better:
def my_function
report_data = prepare_report_data
write_report_data(report_data)
end
def prepare_report_data
customer_data = read(:customer_data, {})
customer_recommendations = read(:substituted_customer_recommendations, '')
report_data = customer_data.merge({customer_recommendations: customer_recommendations})
{ report_data: report_data }
end
def write_report_data(report_data)
filename = 'report_data.json'
result = Result.new(
file: FileIO.new(report_data.to_json, filename)
)
result.save!
end
Write a function instead of repeating code (-> DRY, don't repeat yourself)
bad:
def preview(data)
sanitized = sanitze(data)
write(sanitized)
navigate_to(:preview)
end
def next(data)
sanitized = sanitze(data)
write(sanitized)
navigate_to(:back)
end
better:
def preview(data)
save_data(data)
navigate_to(:preview)
end
def next(data)
save_data(data)
navigate_to(:back)
end
private
def save_data(data)
sanitized = sanitze(data)
write(sanitized)
end
Disclaimer: You should not try to overdo it with DRY. If you have to add many parameters to the new function to make it work for all use-cases it may be better to keep the code separate. Functions with lots of parameters are generally harder to understand.
Write a function instead of asking an object about all its interna: Tell don't ask
This is all about where code should live. If you're asking an object a lot about interna, that's a code smell. It would be better to ask the object to do the things you want for you. Try to refactor that out of your method into its own method in the class where it belongs.
bad:
class MyClass
def create_target(network_profile)
subnet = if network_profile.ip_configured?
IPAddr.new("#{network_profile.ip_address}/#{network_profile.ip_netmask}").to_s
end
name = 'My Target'
Target.create(subnet: subnet, name: name)
end
end
better:
class MyClass
def create_target(network_profile)
Target.create(subnet: network_profile.subnet, name: 'My Target')
end
end
class NetworkProfile
def subnet
if ip_configured?
IPAddr.new("#{ip_address}/#{ip_netmask}").to_s
end
end
end
Classes
If you followed the advice in the first part of this card you now have lots of functions. Those should be structured with classes.
Write service classes
If you have several functions that could be grouped together (e.g. because they are operating on the same data or do something similar), consider adding a service class for them.
good:
class VersionParser
def self.parse_version(version)
version = remove_version_appendix(version)
Versionomy.parse(version)
end
def self.major_version_change?(version_1, version_2)
return false if version_1.blank? || version_2.blank?
parse_major_version(version_1) != parse_major_version(version_2)
end
private
def self.parse_major_version(version)
parsed = version.is_a?(Versionomy::Value) ? version : parse_version(version)
parsed.major
end
def self.remove_version_appendix(version)
version.split('+').first if version
end
end
Write form models
Form models Show archive.org snapshot are not only a nice way to structure your code but also help you get away from callback hell.
Catchy examples for form models are things like user signup or password change forms (note: you should use a library for such things instead of building them yourself. But it is a good example).
class User < ApplicationRecord
attr_accessor :new_password, :new_password_confirmation
validates :new_password, enforce_password_policy: true
validates :new_password_confirmation, presence: true
validates :password, presence: true
before_save :store_new_password
def store_new_password
# ...
end
# ...
end
For most of the time you use the user model these validations and callbacks are useless because they are only used on one screen. So they should move to a form model:
class User::PasswordChange < ActiveType::Record[User]
attr_accessor :new_password, :new_password_confirmation
validates :new_password, enforce_password_policy: true
validates :new_password_confirmation, presence: true
validates :password, presence: true
before_save :store_new_password
def store_new_password
# ...
end
end
Write classes for parameters that belong together
Sometimes you see multiple functions that always use the same parameters. This often happens if the parameters only make sense together - in this case you could add a new class.
bad:
class Geometry
def draw_circle(x, y, radius)
# ...
end
def move_shape(start_x, start_y, end_x, end_y, shape)
# ...
end
#...
end
better:
class Position
attr_accessor :x, :y
def initialize(x, y)
@x = x
@y = y
end
def get_x
@x
end
# ...
end
class Geometry
def draw_circle(position, radius)
# ...
end
def move_shape(start_position, end_position, shape)
# ...
end
end
Namespaces
Use namespaces to structure your classes. Start early, latest when you have 3-4 classes that belong together.
