Seemingly clean code

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!!

Comments

[Swarup Kumar Mahapatra]

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.

[Chakrit Likitkhajorn]

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.