Lava Flow

My favourite antipattern. It starts simply: you can copy-paste code from an old project into the new one. It did something useful there — let it do almost the same in the new project. You just need to comment out one piece and slightly rewrite another.

After about three transfers without refactoring you get large commented-out sections, functions that only work with a subset of their parameters, convoluted workarounds like "let's pour the water out of the kettle and turn off the gas — and that will bring us to the already-known task of boiling the kettle", and so on.

Note: the pattern is deceptively good in the short term and only becomes bad over the long haul. And deadly bad over the very long haul.

Symptoms

  • Variables that appeared out of nowhere.
  • Code fragments unrelated to the task.
  • Very odd documentation coverage, and a lot that looks important beyond it.
  • The architecture is patched together from the previous three architectures.
  • "Who's going to look at it anyway — just hook it into the project!"
  • Obsolete interfaces.

Why it happens

That's if it's the same team. But if the developers on the fifth project are new, the fun begins — this stalactite still has to be read.

Very often I see lava code in outsourcing companies' projects, because they reuse their codebase across different clients as a kind of "innersource". And "cross-disciplinary" code is exactly what overgrows with switchable sections and overridable functions.

In an API-driven approach and Domain-Driven Development this shouldn't happen at all, and we like to think we follow these ideas. More precisely, the architects are sure we do, and then it depends on how things go in each specific team.

Example

The "bad" version: a function carried over from an old project, with dead parameters, commented-out code and a workaround:

def process_payment(payment, account, legacy_flag, retry=False, old_currency=None):
    # legacy: there used to be a mainframe sync here
    # if legacy_flag:
    #     sync_mainframe(payment)
    #     wait_for_response()
    #     apply_compensation(payment)
    if retry and not payment.status == "pending":
        return None
    if old_currency is not None:
        payment.amount = convert_currency(payment.amount, old_currency, "RUB")
    # workaround: balance used to be None after a migration
    balance = account.balance or {}
    account.balance = balance
    account.balance["amount"] = account.balance.get("amount", 0) - payment.amount
    return account

The "good" version: only what belongs to the task, with explicit parameters:

def process_payment(payment: Payment, account: Account) -> Account:
    if payment.status != "pending":
        raise PaymentNotPending(payment.id)
    account.withdraw(payment.amount)
    payment.mark_processed()
    return account

How to fix it

  • Cut out dead and commented-out code entirely — it lives on in version history.
  • Move the code to explicit interfaces and remove vestigial flags.
  • Document only what actually works and is used.
  • If the refactor is too expensive — don't keep moving the branch along; break it down piece by piece based on behaviour (characterization tests).

If a more experienced developer comes up to you and says: "Don't do it that way, I'll tell you how to work around it, that function is outright dangerous, don't touch it unless you have to" — that's exactly it. Such places are a walk through a minefield: you don't know exactly how a code fragment behaves, and some obvious decisions lead to consequences that are interesting to debug.