I am dealing with a pretty big codebase and I was given a few months to refactor existing code. The refactor process is needed because soon we will need to add many new features to our product and as for now we are no longer able to add any feature without breaking something else. In short: messy, huge, buggy code, that many of us have seen in their careers.
During refactoring, from time to time I encounter the class, method or lines of code which have comments like
Time out set to give Module A some time to do stuff. If its not timed like this, it will break.
or
Do not change this. Trust me, you will break things.
or
I know using setTimeout is not a good practice, but in this case I had to use it
My question is: should I refactor the code when I encounter such warnings from the authors (no, I can't get in touch with authors)?
Best Answer
It seems you are refactoring "just in case", without knowing exactly which parts of the codebase in detail will be changed when the new feature development will take place. Otherwise, you would know if there is a real need to refactor the brittle modules, or if you can leave them as they are.
To say this straight: I think this is a doomed refactoring strategy. You are investing time and money of your company for something where noone knows if it will really return a benefit, and you are on the edge of making things worse by introducing bugs into working code.
Here is a better strategy: use your time to
add automatic tests (probably not unit tests, but integration tests) to the modules at risk. Especially those brittle modules you mentioned will need a full test suite before you change anything in there.
refactor only the bits you need to bring the tests in place. Try to minimize any of the necessary changes. The only exception is when your tests reveal bugs - then fix them immediately (and refactor to the degree you need to do so safely).
teach your colleagues the "boy scout principle" (AKA "opportunistic refactoring"), so when the team starts to add new features (or to fix bugs), they should improve and refactor exactly the parts of the code base they need to change, not less, not more.
get a copy of Feather's book "Working effectively with legacy code" for the team.
So when the time comes when you know for sure you need to change and refactor the brittle modules (either because of the new feature development, or because the tests you added in step 1 reveal some bugs), then you and your team are ready to refactor the modules, and more or less safely ignore those warning comments.
As a reply to some comments: to be fair, if one suspects a module in an existing product to be the cause of problems on a regular basis, especially a module which is marked as "don't touch", I agree with all of you. It should be reviewed, debugged and most probably refactored in that process (with support of the tests I mentioned, and not necessarily in that order). Bugs are a strong justification for change, often a stronger one than new features. However, this is a case-by-case decision. One has to check very carefully if it is really worth the hassle to change something in a module which was marked as "don't touch".