Seemingly clean code
Tags: architecture, oop
Published: 2020-08-02 05:06:57

In my career, I have seen many developers prefer or lean toward a code like this.

```Ruby
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.

```Ruby
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.

```Ruby
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.

```Ruby
 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.

```Ruby
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

```Ruby
 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`

```Ruby
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**

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