Comment by nevi-me | Hacker News Reader

Comment by nevi-me | Hacker News Reader

Parent Full thread

> If someone feels compelled to read every function either the functions are poor abstractions or the reader has trust issues, which may be warranted.

I joined a company with great code and architecture for 3 months last year. They deal with remittances and payments.

Their architecture leads are very clued up, and I observed that they spent a lot of quality time figuring out their architecture and improvements, continuously. They'd do a lot of refactors for all the various teams, and the cadence of feature development and release was quite impressive.

In that period though, I and another long-standing colleague made a few errors that cost the company a lot of money, like an automated system duplicating payments to users for a few hours until we noticed it.

Part of their architectural decision was to use small functions to encapsulate logic, and great care and code review was put into naming functions appropriately (though they were comment averse).

The mistakes we committed, were because we trusted that those functions did what they said they did correctly. After all, they've also been unit tested, and there's also integration tests.

If it weren't for the fortitude of the project manager (great guy hey) in firmly believing in collective responsibility if there's no malice, I'd probably have been fired after a few weeks (I left for a higher offer elsewhere).

---

So the part about trust issues resonates well with me. As a team we made the decision that we shouldn't always trust existing code, and the weeks thereafter had much higher cognitive load.

That sounds like a very difficult situation. Would you be willing to elaborate on what kinds of bugs lay in the pre-existing functions? Was some sort of operation that was supposed to be idempotent (“if you call it with these unique parameters over and over, it will be the same as if you only called it once”) not so? I am trying to imagine what went wrong here. A tough situation, must have been quite painful. How serious were the consequences? If you don’t feel comfortable answering that is okay.

I can't remember the exact detail, but one instance was a function checking whether a user should be paid based on some conditions. It checked the db, and I think because the codebase and db move fast, there was a new enum added a few months prior which was triggered by our transaction type.

So that helped function didn't account for the new enum, and we ended up sending >2 payments to users, in some cases I think over 10 to one user.

The issue was brought to customer support's attention, else we might have only noticed it at the end of the week, which I think would have led to severe consequences.

The consequences never reached us because our PM dealt with them. I suppose in all the financial loss instances, the business absorbed the losses.

> So that helped function didn't account for the new enum

This is where Scala/Rust's enforcement of having to handle all arms of a match clause help catch such issues - if you are matching against the enum, you won't even be able to compile if you don't handle all the arms.

Sounds like the source of truth for the enum members may have been in the database.

(But yes, exhaustiveness checking for sum types is a great feature.)

The only db work I've done in rust required a recompile if the db schema changed, or even the specific queries your program used, because the rust types got generated from the schema. So in those cases the db change would have driven a rust type change and rust would have verified exhaustive handling.

Db changes are generally at runtime, how would you recompile rust code during the save of the data to the db? How do you rollback the change if a compile fails? How do you add the necessary code to handle new cases of the enum but not have it present in the db? This is amazingly interesting to me, would love to know more.

Maybe a code gen layer that generates rust types from a db schema. I don’t know rust but have seen those in other languages. I could see a DB enum type corresponding to a language specific enum type and then the language rules applying.

I do think this is a level of indirection myself; if the generated code was perfect and always in sync, that would be one thing, but by definition it is not the case.

Function names aren't wholly distinct from comments. They suffer from the same problems as comments - they can go stale and no longer reflect the code they're naming.

I think the argument against comments is that while function names are a necessary form of communicating intent of the code, comments aren’t. The more forms there are the more work there is to update on each change in the code. Comments mean more to update and hence more to fail to update. They also generally can’t be detected for staleness as well as functions, although that is changing now with better ai, not only compilers etc.

Functions generally need to be documented, especially if there are any gotchas not obvious from the function signature. And one should always read the documentation. Good names are for discovery and recollection, and for the call-site code to be more intelligible, but they don’t replace having a specification of the function’s interface contract, and client code properly taking it into account.

>The mistakes we committed, were because we trusted that those functions did what they said they did correctly. After all, they've also been unit tested, and there's also integration tests.

As it is stated, I don't see where it is your mistake. You should be able to trust things do what they say, and there should be integration testing that happens which adds the appropriate amount of distrust and verification. Even with adequate unit testing, you normally inject the dependencies so it wouldn't be caught.

This seems an issue caused by two problems, inadequate integration testing and bugs in the original function, neither of which are your fault.

Building a sixth sense of when to distrust certain code is something you see from more experienced developers at a company, but you were new so there is no reason to expect you to have it (and the system for making code changes shouldn't depend upon such intuition anyways).

> This seems an issue caused by two problems, inadequate integration testing and bugs in the original function, neither of which are your fault.

I believe the problem may be the culture. Business logic that handles sensitive things where bugs can cost a lot of money is one place where hardcore DRY and factoring everything into small functions is not such a great idea. Yes, it may be a big function, and there is an upfront overhead of having to understand it all to make a change, and there is some duplication of code, but once you understand the function you can reason locally and such bugs would be less likely.

Reply on news.ycombinator.com