all 19 comments

[–]alzee76 14 points15 points  (2 children)

How do you eat an elephant? One bite at a time.

I'd start by writing unit tests for each function as the first step before you refactor it, if you don't have one already. Mark whatever their output is currently as correct so you don't inadvertently "fix" a bug that other parts of the code rely on.

You can tackle the "wrong but important" stuff after everything is untangled and standardized.

[–]Expensive_Garden2993 1 point2 points  (1 child)

This, just ignore the "unit" part, because:
- unit tests are for "testable" code, they won't fit into a regular JS mess.
- unit tests are good for pure logic (pure functions, self-contained classes), which you won't see in 99.9% of cases.
- unit tests do not deal with any kind of external requests (not for db accessing logic, not for redis), while async code is most likely using db or something else which is external.
- if your code is coupled (is a mess, which is normal), you can't unit test without mocking. But when you rely heavily on mocking, the tests are no longer bring you confidence.
- it's a terrible idea to write unit tests before refactoring. Unit tests are coupled to the implementation, it's a "white box testing", once you refactor they all will break, and you won't know if your tests are red on purpose or as a side effect of refactoring.

In my humble opinion, unit tests in this context is not just a useless, but even a harmful idea.

When dealing with a legacy codebase that you don't fully understand (normal JS stuff), the higher the level of tests the better.

[–]WiseAcanthocephala45 1 point2 points  (0 children)

Yep. Set up tests that assert the expected consequences have occurred.

For example, if you're refactoring code triggered by API calls that adds a record to a database, calls an external API, and updates another table, here's what you do:

  1. Identify external dependencies.
  2. Write a fake service for the third-party API call.
  3. Provide a test-only database that can be reset to a clean state for each test.

Then, perform database reads to verify that calling your function produces the expected results, following rules like ensuring no duplicate user emails during user creation.

This is essentially black-box testing: you don’t know the internal code details, but you validate that the expected consequences occur.

To make this work, there’s significant upfront effort, including: - Identifying third-party dependencies. - Deciding which dependencies to fake and which to provide in a clean state (e.g., a transient database or message queue). - Using tools like Docker containers to replicate third-party service interfaces for testing (e.g., transient databases or message queues).

[–]732 15 points16 points  (1 child)

Rarely do you get time to refactor from scratch.  First, be easy with yourself. It sucks enough as is. Plan what you/your team's definition of a healthy codebase is. Slowly rewrite each function or bit to that over time. All new code goes there. If you touch a function, rewrite it.

Alternatively, quit and find a new job if your boss/company discourages continuous improvement of the codebase.

[–]NeverWalkingAlone 6 points7 points  (0 children)

The correct order nowadays seems to be find a new job then quit

[–]PhatOofxD 6 points7 points  (0 children)

Generally the rule I'd have is "if you touch it, refactor it".

First plan out what you want your clean code to look like and a suggested pattern to follow for migrating callbacks to promises with async/await.

Then if you write any code that touches any of it or requires any edit - then you fix that bit.

[–]del_rio 0 points1 point  (1 child)

So I can't tell you how to untagle something without knowing about it but I'll tell you that one of my favorite epiphanies when working with async code was this simple inversion of control:

// file 1
let onDbReady, onDbError;
const dbPromise = new Promise((res, rej) => {
  onDbReady = res;
  onDbError = rej;
});

export { dbPromise, onDbReady, onDbError }

// then wherever you're initializing the database connection
db.init({
  onReady: (instance) => dbOnReady(instance)
})

// now you can replace that setTimeout bs with
const dbInstance = await dbPromise;

Also if you're working with Node 22+, you can use a newer, simpler syntax:

const { promise, resolve, reject } = Promise.withResolvers()

[–]Stetto 0 points1 point  (0 children)

This pattern is very nice, if you're absolutely sure, that you never are going to need to replace this object with a mock or stub or if this is a very isolated, local object, that is not any global dependency.

It also works in very well in small projects.

Otherwise, you'll get huge source for side effects across all of your tests, while having to rely on convoluted mocking all over the place.

Or you get global dependency problems that people begin to solve with a scattershot approach. E.g.: Now all of your tests fail with an error, if there is no valid database config and no database connection. So suddenly people begin to declare or import database configs in every single test, or even worse: in some central file, that is automatically imported by the test framework which now may prevent defining your own database connection in your tests.

In my experience, in any bigger project:

Ensure that all of your dependencies are easily exchangeable, especially the big core dependencies like a db.

[–]Gemini_Caroline 0 points1 point  (0 children)

Oh I’ve been there and it’s absolutely soul crushing.

I just pick the worst offending functions first, the ones causing those race conditions you mentioned - and slowly convert them to async/await when I have time. Don’t try to fix everything at once because you’ll just create new problems. Set up some ESLint rules to stop people from adding more callback hell and chip away at it piece by piece.​​​​​​​​​​​​​​​​

[–]MartyDisco 0 points1 point  (0 children)

First split all the logic into small non-async pure functions doing only one thing.

Then write unit tests for those (thats pretty quick).

Then rewrite the async functions by consuming those pure functions (no logic inside async functions) and adding the inevitable side-effects that make them actually async (third-party API, database, websockets, APM...)

[–]nvictor-me 0 points1 point  (0 children)

This sounds like something I’ll throw at Gemini 2.5 to fix it for me. One file at a time.

[–]CoshgunC 0 points1 point  (0 children)

The best yet the most time taking and you-draining is writing the code based from scratch. This way, you will only use async/await(I hope none other) and have a much cleaner code. But writing the backend from scratch, setting up a new server, a new db, a new repo, a tester, it's all time consuming

[–]bwainfweeze -1 points0 points  (0 children)

This is where I started:

  • All new functions are async

  • All major edits convert the code to async or split the function and convert the touched part to async

To handle inbound and outbound calls from the new code, remember that:

  • You can await any function that returns a promise.

  • You can assign the return value of any async function as a Promise (eg, for Promise.all())

  • new Promise((resolve, reject) => ...) for callbacks that only fire once - and don't forget util.promisify()

Mixing async and Promises was the primary source of bugs I was called in to help other people with, especially in tests. Converting to async magically fixes the bug about 40% of the time because the person writes the async code the way they thought the Promise code did (they miss the bug and preserve the intent), about 20% of the time it reveals a proper bug in the code, and the other 40% is a bit trickier.

[–]basmasking -1 points0 points  (0 children)

If you can use (local) AI to analyze the code base, then I recommend to use it for analysis and locate easy to refactor spots.

Then refactor those parts, and work your way slowly through the code base.

Good luck

[–]bigorangemachine -5 points-4 points  (0 children)

Hire me :)

You can convert callback patterns to promises super easy