[HN Gopher] Mistakes engineers make in large established codebases
___________________________________________________________________
Mistakes engineers make in large established codebases
Author : BerislavLopac
Score : 99 points
Date : 2025-01-07 20:44 UTC (2 hours ago)
(HTM) web link (www.seangoedecke.com)
(TXT) w3m dump (www.seangoedecke.com)
| IvyMike wrote:
| The "The cardinal mistake is inconsistency" is 100% true. We used
| to call the guiding philosophy of working in these codebases
| "When in Rome".
| pryelluw wrote:
| I have this bad codebase at work. Really bad. One of the things
| I've been working on for the past two years is making it
| consistent. I'm almost at the point where interfaces can be
| left alone and internals rewrites in a consistent style.
|
| People often ask why I hardly ever have any prod issues (zero
| so far this year). This is part of the reason. Having
| consistent codebases that are written in a specific style and
| implement things in similar manner.
|
| Some codebases make me feel like I'm reading a book in multiple
| languages ...
| cyco130 wrote:
| > zero so far this year
|
| I saw what you did there.
| onemoresoop wrote:
| Maybe that's not even that bad if number of issues went
| down from multiple a day to none in a couple of days.
| bizzletk wrote:
| > People often ask why I hardly ever have any prod issues
| (zero so far this year).
|
| It also helps that we're still only in January!
| onemoresoop wrote:
| > Some codebases make me feel like I'm reading a book in
| multiple languages ...
|
| In most cases the codebase does consist of muliple languges.
| SoftTalker wrote:
| Yep when I have to work on old code I find something in the
| existing code that's close to what I want to do, and copy/paste
| it. I do not try to abstract it into a common function, unless
| that's already been done and can be used verbatim.
|
| You don't know the 10 years of reasons behind why the code is
| the way it is, and the safest thing is to stay as close as
| possible to how the existing code is written, both to avoid
| landmines, and so that future you (or someone else) has one
| less peculiar style they have to figure out.
|
| All that said, the more usual case is that the code is already
| a huge mess of different styles, because the 100 different
| developers who have touched it before you didn't follow this
| advice.
| layer8 wrote:
| How do you tackle the case where the codebase is consistent in
| a bad way, like pervasive use of antipatterns that make code
| difficult to change or to reason about? If you want to improve
| that, you have to start somewhere. Of course, Chesterton's
| Fence applies.
| aranw wrote:
| I don't like this philosophy as it often leads to stagnation in
| patterns and ways of working that seep into newer systems.
| "That's not how we do things here" becomes a common criticism,
| resulting in systems and services that share the same flaws and
| trade-offs, making progress difficult.
|
| Engineers often adhere too rigidly to these principles rather
| than taking a pragmatic approach that balances existing
| practices with future improvements.
| Cthulhu_ wrote:
| And that's a fair criticism, however, if you change a pattern
| without changing it everywhere, you now have two patterns to
| maintain (the article mentions this). And if multiple people
| come up with multiple patterns, that maintenance debt
| multiplies.
|
| Progress and improvement is fine, great even, but consistency
| is more important. If you change a pattern, change it
| everywhere.
| djeastm wrote:
| >improvements
|
| Therein lies the rub. Everyone has a different idea of what
| is an improvement in a codebase. Unless there's some
| performance or security concern, I'd much rather work in an
| "old" style codebase that's consistent than a continually
| partially updated codebase by multiple engineers with
| different opinions on what an "improvement" is.
| peterldowns wrote:
| I completely agree with this.
| rstuart4133 wrote:
| [delayed]
| protonbob wrote:
| I don't have a real critique because I don't have that many years
| in a codebase the size of OP (just 2). But I struggle with the
| advice to not try and make a clean section of the code base that
| doesn't depend on the rest of the application.
|
| Isn't part of good engineering trying to reduce your
| dependencies, even on yourself? In a latter part of the post, OP
| says to be careful tweaking existing code, because it can have
| unforeseen consequences. Isn't this the problem that having deep
| vertical slices of functionality tries to solve? High cohesion in
| that related code is grouped together, and low coupling in that
| you can add new code to your feature or modify it without
| worrying about breaking everyone else's code.
|
| Does this high cohesion and low coupling just not really work at
| the scale that OP is talking about?
| gleenn wrote:
| It's one thing to reduce dependency and another to have reduced
| consistency. If you have 10 web routes and 1 behaves
| differently, it doesn't matter if the code is cross coupled or
| not, it matters if it behaves similarly. Does it return the
| same status codes on error? Does it always return JSON with
| error messages inside? Do you auth the same way? The
| implementation can be wholly separate but end users will notice
| because logic on their side now has to special-case your 11th
| endpoint because you returned HTTP 20x instead of 40x on error.
| Or when you realize that you want to refactor the code to DRY
| it (Don't Repeat Yourself), now you can't reduce all the
| duplication because you have bespoke parts.
| mbivert wrote:
| I think the gist of it is humility: as a newcomer, you don't
| really know what's out there and why, and there are often good
| reasons for things to be how they are. Not always, but often
| enough for avoiding being too original to be favored. This
| doesn't imply relinquishing on "good engineering habits"
| either.
|
| Now, once you have a deeper understanding of the codebase,
| you'll know when and why to break away from existing patterns,
| but in the beginning phase, it's a good habit to start by
| learning carefully how things are designed and why.
| Salgat wrote:
| Consistency makes code predictable and reduces mental overhead.
| It doesn't mean you have to write it poorly like the rest of
| the codebase, but it does mean using the same general practices
| as the rest of the codebase. Think of it like using knockoff
| legos vs the real thing. They both work interchangeably which
| makes it easy to use them together, but you'd prefer to use the
| nicer lego pieces as much as possible in your builds because
| the material is higher quality, tighter tolerances, just
| overall works better even if it's the same shape as the
| knockoff pieces.
| mrkeen wrote:
| Nope, you've got it.
|
| Code-consistency is a property just like any other property,
| e.g. correctness, efficiency, testability, modifiability,
| verifiability, platform-agnosticism. Does it beat any of the
| examples I happened to list? Not a chance.
|
| > worrying about breaking everyone else's code
|
| You already said it, but just to expand: if you already have
| feature A, you might succeed in plumbing feature B through
| feature A's guts. And so on with feature C and D. But now you
| can't change any of them in isolation. When you try to fix up
| the plumbing, you'll now break 4 features at once.
| hoten wrote:
| I love how the first example is "use the common interfaces for
| new code". If only! That assumes there _is_ a common interface
| for doing a common task, and things aren't just a copy-paste of
| similar code and tweaked to fit the use case.
|
| So the only tweak I'd make here, is that if you are tempted to
| copy a bit of code that is already in 100 places, but with maybe
| 1% of a change - please, for the love of god, make a common
| function and parameterize out the differences. Pick a dozen or so
| instances throughout the codebase and replace it with your new
| function, validating the abstraction. So begins the slow work of
| improving an old code base created by undisciplined hands.
|
| Oh, and make sure you have regression tests. The stupider the
| better. For a given input, snapshot the output. If that changes,
| audit the change. If the program only has user input, consider
| capturing it and playing it back, and if the program has no data
| as output, consider snapshotting the frames that have been
| rendered.
| edudobay wrote:
| Yes, this is the counterpoint I'd make to "resist the urge to
| make every corner of the codebase nicer than the rest of it":
| in an inconsistent codebase, maybe we should prioritize making
| it consistent where possible, and reducing unnecessary
| duplication is one way to reduce future change costs.
| ggregoryarms wrote:
| At some points, new improvement and occasionally ingenuity need
| to find a healthy way back into the workflow. Moreso early on,
| but consistently over time as well.
|
| If we just create copies of copies forever, products degrade
| slowly over time. This is a problem in a few different spheres,
| to put it lightly.
|
| The main rule is a good one, but the article overfocuses on it.
| adamc wrote:
| I really liked this: "as a general rule, large established
| codebases produce 90% of the value."
|
| People see the ugliness -- because solving real problems,
| especially if business practices are involved, is often very
| messy -- but that's where the value is.
| bdangubic wrote:
| I also find amusing that "legacy" more often than not gets used
| in negative conotation. I hear "legacy" and I think "bunch of
| people wrote some AWESOME shit that lasted so long that now
| other people get to view it as 'legacy'"
| tpoacher wrote:
| There's a good chance that's not what people mean by this
| term though.
|
| It's probably used in the (now) classic sense as defined by
| M. Feathers in his "Working with legacy code" book.
|
| Code that is old but otherwise awesome, maintainable (or even
| actively maintained) and easy / a joy to work with are rarely
| referred to as "legacy code".
| Cthulhu_ wrote:
| Earning code trumps pretty code every time.
| nitwit005 wrote:
| Except, the old stuff will be effectively untestable, and they'll
| demand near perfect coverage for your changes.
|
| Also, they're will be four incomplete refactorings, and people
| will insist on it matching the latest refactoring attempt. Which,
| will then turn out to be impossible, as it's too unfinished.
| crabbone wrote:
| OP has some particular type of project in mind, where what they
| say probably makes sense. Not all large codebases are like that.
|
| For example, it could be a lot of individual small projects all
| sitting on some common framework. Just as an example: I've seen a
| catering business that had an associated Web site service which
| worked as follows. There was a small framework that dealt with
| billing and navigation etc. issues, and a Web site that was
| developed per customer (couple hundreds shops). These individual
| sites constituted the bulk of the project, but outside of the
| calls to the framework shared nothing between them, were
| developed by different teams, added and removed based on customer
| wishes etc. So, consistency wasn't a requirement in this scheme.
|
| Similar things happen with gaming portals, where the division is
| between some underlying (and relatively small) framework and a
| bunch of games that are provided through it, which are often
| developed by teams that don't have to talk to each other. But, to
| the user, it's still a single product.
| mjr00 wrote:
| > The other reason is that you cannot split up a large
| established codebase without first understanding it. I have seen
| large codebases successfully split up, but I have never seen that
| done by a team that wasn't already fluent at shipping features
| inside the large codebase. You simply cannot redesign any non-
| trivial project (i.e. a project that makes real money) from
| first-principles.
|
| This resonates. At one former company, there was a clear divide
| between the people working on the "legacy monolith" in PHP and
| the "scalable microservices" in Scala/Go. One new Scala team was
| tasked with extracting permissions management from the monolith
| into a separate service. Was estimated to take 6-9 months. 18
| months later, project was canned without delivering anything. The
| team was starting from scratch and had no experience working with
| the current monolith permissions model and could not get it
| successfully integrated. Every time an integration was attempted
| they found a new edge case that was totally incompatible with the
| nice, "clean" model they had created with the new service.
| cratermoon wrote:
| I worked at a company that had a Rails monolith that underwent
| similar scenario. A new director of engineering brought in a
| half dozen or of his friends from his previous employer to
| write Scala. They formed up a clique and decide Things Were
| Going to Change. Some 18 months and 3 projects later, nothing
| they worked on was in production. Meanwhile the developer that
| was quietly doing ongoing maintenance on the monolith had
| gradually broken out some key performance-critical elements
| into Scala and migrated away from the Ruby code for those
| features. Not only had it gone into production, it made
| maintenance far easier.
| peterldowns wrote:
| As soon as you said Scala I knew that they wouldn't be able
| to deliver anything of any quality, and certainly not on
| time.
| ge96 wrote:
| I'm just thinking about this time at a previous job, I was
| reviewing a PR and they decided to just find/replace every
| variable and switch from snake to camel case. I was like "why are
| you guys doing this, not part of the job". There was some back
| and forward on that. This is a place where PRs weren't about
| reviews but just a process to follow, ask someone to approve/not
| expect feedback.
|
| edit: job = ticket task
| dboreham wrote:
| It's also literally not part of the job.
| Cthulhu_ wrote:
| What was the established code style (...if any) in that
| project?
|
| Anyway it doesn't sound like that was a very mature project or
| developers, not when the reviewer decide to just edit code
| instead of provide a review.
| ge96 wrote:
| the old/existing code was all underscore, they wanted to use
| camelcase instead. it's a dumb thing to be argue about I know
| but it made the code review harder when instead of 10s of
| line diffs there's almost a hundred granted easy to see just
| changing casing
| gwbas1c wrote:
| One thing I did was implement a code formatter, and enforce it in
| CI.
|
| "dotnet format" can do wonders, and solved most serious
| inconsistency issues.
| mrkeen wrote:
| There was only one mistake that the article felt like giving a
| header to: "The cardinal mistake is inconsistency"
|
| The instinct to keep doing things the wrong way because they were
| done the wrong way previously is strong enough across the
| industry without this article.
|
| I love to
|
| > take advantage of future improvements.
|
| However, newer and better ways of doing things are almost
| invariably inconsistent with the established way of doing things.
| They are dutifully rejected during code review.
|
| My current example of me being inconsistent with our current,
| large, established database:
|
| Every "unit test" we have hits an actual database (just like
| https://youtu.be/G08FxxwPjXE?t=2238). And I'm not having it. For
| the module I'm currently writing, I'm sticking the reads behind a
| goddamn interface so that I can have actual unit tests that will
| run without me spinning up and waiting for a database.
| jrockway wrote:
| I like keeping things consistent even if the consistent way is
| "wrong". One thing that bugged me about the large codebase I
| most recently worked on is that we used a custom assert library
| for tests. The Go team says this about them:
| https://go.dev/wiki/TestComments#assert-libraries , and having
| learned Go at Google, I would never have been allowed to check
| in code like that. But this place wasn't Google and there were
| tens of thousands of lines of these tests, so I told new
| developers to keep doing things the "wrong" way. This didn't
| cause many problems, even if failing tests failing too soon is
| pretty annoying. Most of the time the tests pass, and the
| yes/no signal is valuable even if you can debug more by simply
| `t.Errorf(...)` and continuing.
|
| As for starting databases during tests, it's saved me a lot of
| trouble over the years. One time, we used sqlite for tests and
| Postgres for production. We had some code that inserted like
| `insert into foo (some_bool) values ('t')` and did a query like
| `select * from foo where some_bool='true'`. This query never
| matched rows in the tests, because t != true in SQLite, but t
| == true in Postgres. After that, I found it easier to just run
| the real database that's going to be used in production for
| tests. The only thing that behaves identically to production is
| the exact code you're running in production.
|
| Over here, I have code that uses a hermetic Postgres binary
| (and chain of shared libraries because Postgres hates static
| linking) that starts up a fresh Postgres instance for each
| test. It takes on the order of a millisecond to start up: https
| ://github.com/jrockway/monorepo/blob/main/internal/test.... The
| biggest problem I've had with using the "real" database in
| tests is low throughput because of fsync (which `perf` showed
| me when I finally looked into it). Fortunately, you can just
| disable fsync, and boy is it fast even with 64 tests running in
| parallel.
|
| One thing that's been slow in the past is applying 50
| migrations to an empty database before every test. When you
| have one migration, it's fast, but it's one of those things
| that starts to slow down as your app gets big. My solution is
| to have a `go generate` type thing that applies the migrations
| to an empty database and pg_dumps resulting database to a file
| that you check in (and a test to make sure you remembered to do
| this). This has two benefits; one, tests just apply a single
| SQL file to create the test database, and two, you get a diff
| over the entire schema of your database for the code reviewer
| to look at during code reviews. I've found it incredibly useful
| (but don't do it for my personal projects because I've been
| lazy and it's not slow yet).
|
| Overall, my take on testing is that I like an integration test
| more than a unit test. I'd prefer people spend time on
| exercising a realistic small part of the codebase than to spend
| time on mocks and true isolation. This is where a lot of bugs
| lie.
|
| Of course, if you are writing some "smart" code and not just
| "glue" code, you're going to be writing a lot of unit tests.
| Neither replaces the other, but if you can spend 30 seconds
| writing a test that does actual database queries or 2 weeks
| mocking out the database so the test can be a unit test instead
| of an integration test, I'd tell you to just write the
| integration test. Then you know the real code works.
| Cthulhu_ wrote:
| If it's _wrong_ then it needs to be fixed, obviously, but only
| if you fix it in a way that ensures consistency and doesn 't
| break existing functionality. But the article doesn't mention
| _wrong_ code per se, just different code. There 's always
| multiple ways to solve a problem, stick to one for you and the
| 999 other developers' sakes.
|
| Your example is a good example; you call it a unit test, but if
| it hits a real database it's by definition an integration test.
| No mocked database will be as accurate as the real deal. It'll
| be good enough for _unit_ tests (amortize / abstract away the
| database), but not for an integration test.
| o_nate wrote:
| A big part of this advice boils down to the old adage: "Don't
| remove a fence if you don't know why it was put there." In other
| words, when making changes, make sure you preserve every behavior
| of the old code, even things that seem unnecessary or counter-
| intuitive.
| Aloha wrote:
| Chesterton's Fence
| gwbas1c wrote:
| Unit tests, exhaustive regression tests, and automated tests are
| the best way to prevent regressions.
|
| Time spent writing good unit tests today allows you to make
| riskier changes tomorrow; good unit tests de-risk refactors.
| notyourwork wrote:
| Unit tests cover the single functionality but ignore the system
| as a whole. Most regressions I've seen in industry are because
| of a lack of understanding how the system components interact
| with one another.
|
| Therefore, I see unit tests as one pillar but also suspect that
| without good quality integration or end-to-end testing you
| won't be able to realize the riskier re-factors you describe.
| Perhaps you consider these part of your regression testing and
| if so, I agree.
| peterldowns wrote:
| I agree that consistency is important -- but what about when the
| existing codebase is already inconsistent? Even worse, what if
| the existing codebase is both inconsistent _and_ the "right way
| to do things" is undocumented? That's much closer to what I've
| experienced when joining companies with lots of existing code.
|
| In this scenario, I've found that the only productive way forward
| is to do the best job you can, in your own isolated code, and
| share loudly and frequently why you're doing things your new
| different way. Write your code to be re-used and shared. Write
| docs for it. Explain why it's the correct approach. Ask for
| feedback from the wider engineering org (although don't block on
| it if they're not directly involved with your work.) You'll
| quickly find out if other engineers agree that your approach is
| better. If it's actually better, others will start following your
| lead. If it's not, you'll be able to adjust.
|
| Of course, when working in the existing code, try to be as
| locally consistent as possible with the surrounding code, even if
| it's terrible. I like to think of this as "getting in and out" as
| quickly as possible.
|
| If you encounter particularly sticky/unhelpful/reticent team
| members, it can help to remind them that (a) the existing code is
| worse than what you're writing, (b) there is no documented
| pattern that you're breaking, (c) your work is an experiment and
| you will later revise it. Often asking them to simply document
| the convention that you are supposedly breaking is enough to get
| them to go away, since they won't bother to spend the effort.
| mgfist wrote:
| I rarely see large 10m+ LOC codebases with any sort of strong
| consistency. There are always flavors of implementations and
| patterns all over the place. Hell, it's common to see some
| functionality implemented multiple times in different places
| peterldowns wrote:
| And it's fine, right? Honestly I think people need to realize
| that part of being a good engineer is being able to deal with
| inconsistency. Maybe submodule A and submodule B do network
| requests slightly differently but if both ways are
| reasonable, working, and making the company money, it's
| probably not worth delaying product improvements in order to
| make things "more consistent."
|
| On the other hand if _no one in your company_ cares about
| consistency, at some point everything becomes so awful you
| basically won 't be able to retain engineers or hire new
| ones, so this is a place where careful judgement is needed.
___________________________________________________________________
(page generated 2025-01-07 23:00 UTC)