Code Smells by Coupling
Coupling is the extent to which any component of your software depends on other components. When software starts to mature, the coupling decisions neglected early on in development can come back with a vengeance and severely slow down what should be a very simple update. Making all aspects of your code as independent as possible will go a long way in keeping technical debt to a minimum and maintainability to a maximum as the code base matures.
Parameters of Methods
The most common code smell by coupling I see is passing multiple parameters into a method. I prefer to pass in a hash and use the hash keys for my variables in the method.
Bad:
def some_method(param1, param2, param3, param4) # do some stuff end
Good:
def some_method(my_params) # do some stuff end
If there are optional keys in this hash, you can check for them inside the method. This will save you headaches when the method parameters inevitably change, and you now have to update the method call all over the application (don't forget your .send or dynamic method calls!).
I know some of you are asking: "What about type hinting?". My response to you is to write better comments. Every method should be documented with the parameter types, as well as what the parts of those parameters are, should that parameter be something such as a hash.
Passing the Params Hash From a Form into a Method
Now we can move onto a few less obvious coupling smells. The most frequent of these is passing the params hash into a method. Lets say you have an instance method which manipulates the object given the values submitted from a form.
It's very tempting to say: "I'm just going to pass params into this method, what could go wrong?". On the surface, it seems like a good idea. The method only has one parameter. Ruby is all objects so it will pass the hash by reference, so I'm not duplicating the hash in any way. Params has a few extra fields I don't need, but so what? It's convenient!
Bad:
def self.new_instance(params) new_val = params[:weird_field_name_1] + params[:weird_field_name_2] Object.new(field_1: new_val, field_2: params[:weird_field_name_3], field_3: params[:weird_field_name_4]) end @instance = Instance.new_instance(params)
The reason passing in params causes a code smell is because you've now tied an instance method to the naming convention used on a view. This instance method isn't very reusable. I know you are already saying to yourself, "But all the other forms using the same data should have the same names for all the fields".
The thought that every form manipulating this data will have the same structure and naming convention is very idealistic. Also, what happens if you want to do the same manipulation to the instance, but the information doesn't come from a form? Now you have to make a hash using the same naming convention the view used that might not make sense in your use case. It stinks in here!
The answer? Make your own hash that makes sense (Because passing multiple parameters into a method is a coupling code smell), containing only the information you really need. Bite the bullet and pull what you need out of submitted params and put it into your instance parameter hash. Remember, less code isn't always better code. Clear and maintainable code is the best code. You now have a reusable, decoupled, and maintainable, instance method.
Good:
def self.new_instance(my_params) new_val = my_params[:universal_field_1] + params[:universal_field_2] Object.new(field_1: new_val, field_2: params[:universal_field_3], field_3: params[:universal_field_4]) end my_params = { universal_field_1: params[:weird_field_name_1], universal_field_2: params[:weird_field_name_2], universal_field_3: params[:weird_field_name_3], universal_field_4: params[:weird_field_name_4]} @instance = Instance.new_instance(my_params)
The Law of Demeter
The next most common smell by coupling I see are Law of Demeter violations. The short and skinny of the Law of Demeter is you shouldn't directly reference other objects in your classes. Lets say we have an invoice and a client. A Client has_many :invoices and an Invoice belongs_to a :client
class Invoice < ActiveRecord::Base belongs_to :client end class Client < ActiveRecord::Base has_many :invoices end
Lets say we want to have a way to get the client name and address from the Invoice class. It might be tempting to say:
class Invoice < ActiveRecord::Base belongs_to :client def client_name client.name end def client_address client.address end end
However, you have now coupled the Invoice class to the Client class. If the client instance goes away, this will throw an error because you can't call .name on nil.
Enter rails delegate! From my view, the real power of delegate lies in the allow_nil flag. Lets see how that flag gets us around the hard nil error.
class Invoice < ActiveRecord::Base belongs_to :client delegate :name, :address, :to => :client, :prefix => :client, :allow_nil => true end class Client < ActiveRecord::Base has_many :invoices end
We can now call @invoice.client_name
Two things to notice here:
1) We don't need helper methods on Invoice to get the client name and address
2) Since we used allow_nil, If client doesn't exist, we get nil returned instead of an error:
Good (with allow_nil):
@invoice.client_name => nil
Bad (without allow_nil):
@invoice.client_name => RuntimeError: Child#user_friend_ids delegated to user.friend_ids, but user is nil
Automatic rails methods and less chance of a hard error? Smells like roses!
So, what can you easily do decouple your code?
Three things:
1) Pass in hashes instead of multiple parameters to your methods.
2) Don't pass the Params Hash in as a parameter.
3) Respect the Law of Demeter, it's the law!
If you have figured out a way to get delegate to work from the has_many side (that doesn't require lots of lines of code), please leave a comment describing your methodology.