php.hospital
Act: Refactoring | php.hospital

Refactoring

Refactoring is the process of changing a software system in such a way that it does not alter the external behavior of the code yet improves its internal structure.

Don't refactor all at once

Refactoring is a process that should be done incrementally. It might be tempting to refactor (or even rewrite) a whole system at once, but then you work on it for 2 months and the customer gets no value out of it. No new features, no bugfixes, nothing (at least in their eyes).

When possible, try to take chunks that you can refactor. Or better yet, when you're working on a new feature, refactor the code that is in touch with the new feature. This way, you're not only building a new feature, but you're improving the codebase as a whole.

Make refactoring part of your daily work and make it a habit. Don't create a big agile story or even epic and then work on it for weeks (or months!). If you do a big cleanup but don't practice it regularly, the codebase will just get dirty again. This way, you don't need to get "permission" from the product owner or project manager or head of software engineering. It's just part of your work.

There are exceptions to this rule. Especially when you're working on a new (or at least newly acquired / adopted) project, it might be a good idea to agree on architectural decisions and some fundamentals. And this might require a small refactoring phase to get the codebase to a state where you can work with it. Otherwise, when you move lots of files around, structure the directories and rename classes, you might get merge conflicts all over with people who are trying to add features.

Test before you refactor

"Well, the setup is so complicated that I can't write a test for it." Well, that's an even better reason to write a test. It's often the first test that's hardest to write. You need to setup a test infrastructure first, you need to make sure you have enough data to test anything. And for the first test, I try not to focus on just a small unit test (maybe as a warmup), but really try one that "cuts through" all the layers of the system. From the input, to the controller, entity / model, database, some processing and returning something. This can be gruelsome work, but once you have a working setup, it will become that much easier to write more tests.

Make sure the tests are infinitely repeatable and "refresh" each time. I've had projects where the database would not be reset, so the "unique" constraint in a column might fail. I've seen tests where it is dependent on an external service, so when that external service was down, the tests would fail. Don't be like that. Make sure to make them repeatable. Make sure they're fast (otherwise it's not fun to re-run frequently). Make sure they're not too brittle and fail for the wrong reasons.

Got a bug? Reproduce the bug, test it, then fix it.

Tests are not some additional work that you begrudgingly have to do because someone wrote it in a book. Tests should give you confidence that your code works. Tests should give you confidence that you actually fixed the bug! Tests should give you peace of mind that you didn't inadvertently break something.

So if you get a bug report, reproduce it first. You should write a test and hopefully your reaction is "Yep, I just triggered the exact same error. And this new test which tests the behaviour we actually wanted is failing because of it." And then you fix the bug and make the new tests run green. And then you can confidently say "Yep, I creating a test that failed because of the bug you reported, but now it's fixed."

"Break production once, shame on you.
Break production twice, shame on me."

Coder talking to a bug

If you don't use it, uninstall it

This doesn't fall directly under refactoring, but it's a good practice to keep your codebase free of unused stuff.

I think "Installing it just so we have it" is worse than not installing it all. It will just confuse others ("is this being used?", "are we planning to use it?") and it adds extra work, because you need to keep it up to date, otherwise it might keep other modules from being updateable or you can risk security issues.

The city of Cologne (Germany) has their own 11 "Basic Laws". Number 6 is

"Kenne mer nit, bruche mer nit, fott domet"

Nr. 6 of Cologne's 11 "Basic Laws"

which translates to "Don't know it, don't need it, get rid of it". And I think it's usable here. If you have no idea what a module does, you can't find where it's being used and after asking around, nobody knows what it's for, then it's time to get rid of it.

Just be very sure it's something you don't need. So make sure that your code is at least partially covered by tests. And remember Chesterton's Fence: "Don't remove a fence until you know why it was put up in the first place." Or to be more applicable to software development: "Don't remove a piece of code until you know why it was put there in the first place." And if you understand why it's there, then you can make a decision to remove it or not.

But it's such a great feeling to get rid of thousands of unused files that are just cluttering up your project. It's like /r/oddlysatisfying, but for code!

« Act: Use external services where you can
Act: Typehints, Constants, Enums »