In my career, I have seen many developers prefer or lean toward a code like this.
class Invoice
def pay
make_sure_invoice_approved
create_transaction_entry
decrease_company_total_balance
record_tax_deduction
end
def approve
# ....
end
def create
# ....
end
def cancel
# ....
end
end
At first glance, this code seems clean and easy to follow. You can read it as simple plain English phrases. Every step to make sense. Nothing clutter. Especially in Ruby, it looks even neater because the code doesn't have any parenthesis.
Many developers considered this code neat and clean, and I found many developers try to refactor their code to look like this.
But when I look closer, the implementation of each step may look like this.
private
def make_sure_invoice_approved
@invoice = Invoice.find(@id)
raise Error if !@invoice.approved?
end
def create_transaction_entry
@transaction = Transaction.process(@invoice)
end
def decrease_company_total_balance
@invoice.company.balance = @invoice.company.balance - @transaction.amount
@invoice.company.balance.save()
end
def record_tax_deduction
TaxEntry.record(@transaction)
end
Note: From any reader who is not familiar with Ruby, @invoice
is equivalent to this.invoice
or self.invoice
in other OOP languages.
After dig down to the implementation, I would argue that the original pay
method is not clean at all. The plain-simple-English sentences coding style is not something we should prefer.
Why? Let me explain more.
Implicit dependencies
Let's start with by simple scenario: What if a new junior developer or stakeholder comes to your desk and asks, "How do we decrease company balance after an invoice is paid?"
So now, you look into this method.
def decrease_company_total_balance
@invoice.company.balance = @invoice.company.balance - @transaction.amount
@invoice.company.save()
end
Ok. Now you know that decrease a balance from the company of @invoice
by @transaction.amount
.
But where did @invoice
come? How did we get a @transaction
?
This method does not tell any of it at all. You need to look into the whole Invoice
class to answer.
Now if Invoice
has only a single pay
method, you can still track it. But what if other methods such as approve
, create
, or cancel
also mutate @invoice
and @transaction
?
Now you need to read the whole class to be 100% sure that @invoice
comes from which step.
And if bug happened around this method, now aside from debugging this method itself, you need also to make sure no other methods in class or caller accidentally mutate @invoice
or @transaction
.
Let's compare to the code below.
def pay
invoice = fetch_approved_invoice
raise InvoiceInvalidError if invoice.blank?
transaction = create_transaction_entry(invoice)
decrease_company_total_balance(invoice.company, transaction)
record_tax_deduction(transaction)
end
This code is not a plain simple English anymore. But you can now answer to junior devs and stakeholders with 100% confidence that "We decrease balance based on the company of the approved invoice and the amount inside a transaction created from it".
You got a clearer picture on how it work and how each methods are wired together. I would argue that this is more "Readable" than the previous version, given that reader is a programmer with a basic knowledge of how variable and method call work, which is very very basic.
Changeability
Let's get back to the original code.
class Invoice
def pay
make_sure_invoice_approved
create_transaction_entry
decrease_company_total_balance
record_tax_deduction
end
And what if stakeholder request you to create multiple transactions for any installment invoice.
Now the naive way to achieve this is to look into create_transaction_entry
and create multiple transactions there.
That makes perfect sense. The method itself should be responsible for creating transaction. So we should be able to change this method and call it a day, right?
No!! Because all methods below secretly depends on an amount this method saved into @transaction
.
Compare to this version
def pay
invoice = fetch_approved_invoice
raise InvoiceInvalidError if invoice.blank?
transaction = create_transaction_entry(invoice)
decrease_company_total_balance(invoice.company, transaction)
record_tax_deduction(transaction)
end
The dependency of the below methods are more apparent. You know that you cannot merely create more transactions and call it a day.
When dependencies are implicit, the effect of the change is not.
Next case: Let say if you want to insert a step between makre_sure_invoice_approved
and create_transaction_entry
class Invoice
def pay
make_sure_invoice_approved
# We want to do something here
create_transaction_entry
decrease_company_total_balance
record_tax_deduction
end
Now you need to audit every line of code in create_transaction_entry
, decrease_company_total_balance
, record_tax_deduction
, and make sure that any code you insert will not affect methods below.
So, we make a code neater by hiding complexity. And hence, the code is harder to change, extend, and debug.
Methods communication
At this point, you might ask: But then do you suggest we get rid of internal state and always communicate via a variable?
Well, let's not be so black and white.
I think there are use-cases for methods to communicate via class field. And that is "External call".
Let's say I have public methods
class Invoice
def pay
approve(@creator) unless approved?
raise PermissionNotAllowed if !@approver.can_pay?
end
def approved?
@state == "Approved"
end
def approve(approver)
@state = "Approved"
@approver = approver
save
end
end
If I read just this class, I cannot guarantee previous state of @state and @approved since it can be mutated by external call.
So this is very different from the first case, where I can guarantee @invoice
, @transaction
because it can only mutated by internal private methods.
And when stakeholders ask about "how pay work?". You can simply answer that if Approver of the invoice does not have permission, we don't allow to pay.
And where Approver come?
Well, you can say that it depends on the history of the invoice. (which basically translate to "External call" in technical perspective). You cannot avoid the fact that this class doesn't have full control over @approver
. That is the truth.
I would not say I always get this right. But my simple rule of thumb is: Allow methods to communicate via internal fields if public methods can mutate those fields.
That is my simple initial rule. I cannot limit input spaces for these fields anyway, so I don't bother it much.
So in principle, whenever I can limit input space for method, I do. I stop using implicit dependency because it increases input space. And when I cannot, I just accept.
Please again: This is not a hard rule. Don't blindly follow it. It is just a good start point to think about.
I think this also related to why in OOP paradigm, small class is prefer. Because if you limit everything to a bunch of small classes, you can just ask every developer to read the whole class before modify any method.
But I want to go further, and I say we can make a method inside a class editable with just reading the signature and expected input-output of the method if we are mindful about dependency.
I write this article to argue that a code in plain-and-simple English is not always neat, beautiful, or clean. And it is not the goal that we, as a coder, should strive for.
It can make code very hard to follow, hard to change, hard to debug, and inextensible. And what do you get back? Bunch of code as English phrases or sentences.
Is it easier to read? Maybe. But I would say again that even a mediocre programmer with basic knowledge of how variable work and method call work, can read code with explicit dependencies.
I don't think that worth it.
Thanks for reading!!
Top comments (2)
Great article.. the obsession of OOPs leads to creating such kind of code which has lot of implicit dependency.
The God Object is what causes this.
Thanks.
I think the God object cause this. But to look further, I believe the tendency to create a god object happened because we need to implement a change that require some information or variable that exists in other object/method.
And the easiest way to allow new change to access that information, is to put that information in the scope that is accessible from both current method and new method.
Hence: God object that includes every changes made, communicate by implicit dependency using private fields.
The better way to do this is to be mindful and put a thought into how should we pass this information around.
One thing I learned about software engineering in general is: Be mindful about communication flow between subsystems. Make it as simple and clear as it can be.
This apply to every level of architecture. This principle is valid from high level architecture such as 3rd-party integrations, Microservices, Event-Driven design - to granular level design such as Objects and Methods.