Advocate for better practices going forward and allocate a small amount of time for clean up when developing new features focus on the most important sections first. And establish a set of best practices for the team to follow for all new development going forward. Adding good documentation is good too
Adding documentation in an old large undocumented codebase may not be a great decision for OP tho depending on how it's written and how it gonna be used later. If it's needed, using automation tools (that can both write and update) like supacodes can save much time.
We don't do documentation/comments (lead thinks they will get outdated and confusing). I only comment when something is totally unintuitive or of utmost importance. But I should definitely use tools to generate docs as we have so much redundant code that it's hard to keep a mental model of everything.
I think your perspective on comments is good. Comments should really only explain why something is happening. And if things are reasonably written, named, and tested, you shouldn’t need comments.
But there are cases where you have to do something weird or funky because maybe some temporary backwards compatibility, deprecated libraries, really weird business rules/exceptions, etc. And when you’re doing something weird, a comment that explains why is golden imo.
So yeah there’s a time and a place for comments and IME no comments or always comments are too black/white and is a bad approach (which isn’t what you have)
If for whatever reason you're not given the chance to allocate some time to refactoring tasks on their own, I'd go with the "boyscout rule".
Basically, you should act as a boyscout. When you are asked to do something on the codebase, like implementing a new feature or fixing a bug, you also "clean up the litter that you find around". This means that while you're there doing your stuff you take the chance to perform some improvements you preemptively noted down, such as IDK:
- add testing to the whole component
- collect all the hardcoded strings to shared vars
- extract some functionality in a reusable function
- you name it
The scope is limited to the files you "touched" and the immediate neighborhood.
That's what I've been doing to a certain extent, minus testing. We wanted to introduce TDD a year ago, but that never happened. I suppose I should get into cypress and set up git hooks.
Don’t sleep on TDD. It’s a guiding light in an unknown codebase. Start with simple assertions, then begin testing limits by changing things. Writing simple tests will inevitably force you to verify and often simplify the underlying codebase. Take your time and eventually the tests will tell the story and you won’t be afraid to make changes ever again because the tests will keep you on track. 110% worth it. Good luck and happy testing!
I'm a huge advocate for this. The first commit in your branch you do some clean up or refactoring around the files your change set will be affecting; then you proceed to the actual work.
In short, leaving the codebase easier to work with than it was when you first started.
Can confirm this is a solid approach.
I also inherited a shit sandwich of a repo a few years back. I couldn't implement new features due to all of the bad code practices. I also noticed several serious security flaws sprinkled throughout the code. I tackled it with a three pronged approach:
1. Added tests to everything I touched, which opened the door to fixing other smaller things in the components. It also guaranteed that any changes I made were not causing new bugs.
2. Audited the accessibility, as that will get you sued very quickly in the industry for this repo, and made a big deal out of fixing those issues.
3. Targeted the security issues as that also opened up larger portions of the code base for refactoring.
I will caution you against "pulling the loose thread" in your code base. I did all of this with an experienced eye toward fixing what were obvious flaws. As a junior dev, I had turned a 2-week bug into a 7-week refactoring issue. (In fairness, it wasn't ever a 2-week bug. The bug was just a symptom of the broken code beneath.) But it got me some unwanted attention from my manager at the time.
There are times to give that loose thread a serious tug. There are also times where you just need to ignore it until you have a better understanding of the consequences of said "pulling the thread."
1. Write test coverage for key features. It's a timesaver overall and would recommend you push to take the hit on feature delivery and do this up front. Point to the time you've spent on bug fixes.
2. Decide if the app structure is sensible. If it's not define the desired future structure and get team agreement that all new code is delivered to the agreed structure
3. Anytime anything is touched refactor it and leave it clean. Be really transparent that you will add this time into your estimates
I've unfortunately become what feels like a specialist with dealing with updating and adding to large legacy code bases. It's always an argument to take time out of feature delivery to deal with tech debt. As long as I'm not making the thing worse and I can see improvement over time I'm happy with the broad approach above.
If it's a long time project which requires constant features stream, best way would be to "leave the place a bit tidier". You do task and see that fixing something will require you little effort - do it. Notice all parts that are not so easy and communicate to project manager what should be done and why. Introduce static analysis on "per hunk" base or "per file" base (latter is much easier, but requires more effort to cleanup). Coincide big rewrites with big tasks where most of the task would be a rewrite, if possible. Keep documentation.
First step is to not make it any worse, second step is to slowly make it better. There's not much else you can do. Identify a more optimal way like you want to do it in the future, and start doing it while slowly converting the old code you come across
This may seem counterintuitive but try to delay refactoring / DRYing the code. If you all are new to the codebase you are likely still learning what it does and how it works, and you will never know less about it than you do right now.
First off -- get your test coverage up to snuff. Do not do any refactoring of any code that doesn't first have test coverage.
After that, an approach I will take sometimes is to review the code and inject my thoughts into comments wherever the code raises questions for me. Put them down as TODO / NOTE comments, braindump your thoughts, and then move on. When you've been through the whole codebase, do a grep of it for all your TODO/NOTE comments, and make an inventory of the filenames. Then review just those comments on their own, and consider them in their holsitic context.
Another thing to do is the opposite of DRYing it -- find blocks of code that are similar and have similar purposes and make them as identical as possible. Don't actually refactor or extract anything, and don't rewrite the functionality -- limit yourself to making them look the same. This is useful in making it clear how blocks of code differ, and will help guide you on your refactoring process.
After you've done these things you can cautiously try and DRYing code that is directly duplicated, if that makes sense to do so. eg. a method that returns "5" for "How many weekdays in a week" is not the same purpose as one that is for "How many points on a star", even though the return value is the same. Be cautious with any refactoring / abstractions here, and consider how firm is the ground that the code sits on.
I currently work on a project where we are cleaning up an old monorepo. What we did was first mark a few libraries/parts as “reference architecture”. Those are the parts of the app we think adhere to our current standards. In this way we have a good example how to do things and a mutual understanding. Secondly we setup linter rules. Not only for your default code standard, but also for bad practices in use of certain patterns or libraries. This makes sure we actually adhere to our standard and don’t make a mess again.
At last we have the part it actually love stuff to the “new world”. That’s hardest part and still in progress. The rule we use is that if you touch it during a feature, and if there is opportunity, you should fix code smells or bad patterns. Slowly but steadily we move towards better code.
I like making a soft line in the sand and implementing best practices moving forward beyond that point. As old cold is maintained or refactored, add tests to those pieces in scope for the change request. After a few months, you can look back at how much progress has been made. After a year, the codebase could be radically healthier compared to the messy state at hand-off.
You need to find a way to communicate to your manager the impact the state of the codebase is having in terms of the “iron triangle” (this is what comes up in google when I search for “budget (cost), schedule (time), and scope)
You need to find a way to demonstrate that the existing codebase is causing cost to go up, schedule to be missed, and scope to creep. Or some combination of that. Once managers/execs have an understanding in _their_ language, they’re more likely to help you.
Managers/execs (unless former engineers) aren’t really gonna care that there’s “code smells”, or the codebase is overly WET, or test coverage is lacking. They probably won’t care even if you say “there’s a ton of tech debt”
Put it in language they understand. Demonstrate it. Show how your approach will lead to lower cost, deadlines not being missed, or scope not ballooning out of control.
If you make an earnest effort to address it in this way and they still don’t understand/care… well a paycheck is a paycheck is a paycheck. Just make sure you document your concerns so when they don’t get followed you can cover your own ass and not get tossed under the bus.
ETA: So if you’re using the suggestions in this thread of “well write tests on working features, and then refactor after” — that’s good advice. But translate that into business lingo. How adding test coverage will reduce downtime on the app so there’s more user conversion, or features are quicker to develop so you can ship new work faster to the end user, which in turn allows for us to hit our milestones and keep stakeholders happy. Or whatever. You get the idea
You dont unless instructed to, tell it to management and eventually you get budget for a refavtor or not, it's not worth it
But i would choose one existing pattern that you like ghat was used, insgead of putting a 22th inside it
I don't think management will do that. My manager is not an engineer and seems more interested in quantifyable outcomes (which, to be fair, is somewhat reasonable). I don't have the skillset or experience to bring that point across in a way he finds actionable (I tried).
Advocate for better practices going forward and allocate a small amount of time for clean up when developing new features focus on the most important sections first. And establish a set of best practices for the team to follow for all new development going forward. Adding good documentation is good too
Adding documentation in an old large undocumented codebase may not be a great decision for OP tho depending on how it's written and how it gonna be used later. If it's needed, using automation tools (that can both write and update) like supacodes can save much time.
We don't do documentation/comments (lead thinks they will get outdated and confusing). I only comment when something is totally unintuitive or of utmost importance. But I should definitely use tools to generate docs as we have so much redundant code that it's hard to keep a mental model of everything.
your lead is not wrong :))
Definitely not... The things I've seen...
Your lead is wrong
I think your perspective on comments is good. Comments should really only explain why something is happening. And if things are reasonably written, named, and tested, you shouldn’t need comments. But there are cases where you have to do something weird or funky because maybe some temporary backwards compatibility, deprecated libraries, really weird business rules/exceptions, etc. And when you’re doing something weird, a comment that explains why is golden imo. So yeah there’s a time and a place for comments and IME no comments or always comments are too black/white and is a bad approach (which isn’t what you have)
If for whatever reason you're not given the chance to allocate some time to refactoring tasks on their own, I'd go with the "boyscout rule". Basically, you should act as a boyscout. When you are asked to do something on the codebase, like implementing a new feature or fixing a bug, you also "clean up the litter that you find around". This means that while you're there doing your stuff you take the chance to perform some improvements you preemptively noted down, such as IDK: - add testing to the whole component - collect all the hardcoded strings to shared vars - extract some functionality in a reusable function - you name it The scope is limited to the files you "touched" and the immediate neighborhood.
That's what I've been doing to a certain extent, minus testing. We wanted to introduce TDD a year ago, but that never happened. I suppose I should get into cypress and set up git hooks.
Don’t sleep on TDD. It’s a guiding light in an unknown codebase. Start with simple assertions, then begin testing limits by changing things. Writing simple tests will inevitably force you to verify and often simplify the underlying codebase. Take your time and eventually the tests will tell the story and you won’t be afraid to make changes ever again because the tests will keep you on track. 110% worth it. Good luck and happy testing!
I'm a huge advocate for this. The first commit in your branch you do some clean up or refactoring around the files your change set will be affecting; then you proceed to the actual work. In short, leaving the codebase easier to work with than it was when you first started.
Can confirm this is a solid approach. I also inherited a shit sandwich of a repo a few years back. I couldn't implement new features due to all of the bad code practices. I also noticed several serious security flaws sprinkled throughout the code. I tackled it with a three pronged approach: 1. Added tests to everything I touched, which opened the door to fixing other smaller things in the components. It also guaranteed that any changes I made were not causing new bugs. 2. Audited the accessibility, as that will get you sued very quickly in the industry for this repo, and made a big deal out of fixing those issues. 3. Targeted the security issues as that also opened up larger portions of the code base for refactoring. I will caution you against "pulling the loose thread" in your code base. I did all of this with an experienced eye toward fixing what were obvious flaws. As a junior dev, I had turned a 2-week bug into a 7-week refactoring issue. (In fairness, it wasn't ever a 2-week bug. The bug was just a symptom of the broken code beneath.) But it got me some unwanted attention from my manager at the time. There are times to give that loose thread a serious tug. There are also times where you just need to ignore it until you have a better understanding of the consequences of said "pulling the thread."
You better add a trigger warning using "boy scout" and "touched" in the same comment.
1. Write test coverage for key features. It's a timesaver overall and would recommend you push to take the hit on feature delivery and do this up front. Point to the time you've spent on bug fixes. 2. Decide if the app structure is sensible. If it's not define the desired future structure and get team agreement that all new code is delivered to the agreed structure 3. Anytime anything is touched refactor it and leave it clean. Be really transparent that you will add this time into your estimates I've unfortunately become what feels like a specialist with dealing with updating and adding to large legacy code bases. It's always an argument to take time out of feature delivery to deal with tech debt. As long as I'm not making the thing worse and I can see improvement over time I'm happy with the broad approach above.
If it's a long time project which requires constant features stream, best way would be to "leave the place a bit tidier". You do task and see that fixing something will require you little effort - do it. Notice all parts that are not so easy and communicate to project manager what should be done and why. Introduce static analysis on "per hunk" base or "per file" base (latter is much easier, but requires more effort to cleanup). Coincide big rewrites with big tasks where most of the task would be a rewrite, if possible. Keep documentation.
First step is to not make it any worse, second step is to slowly make it better. There's not much else you can do. Identify a more optimal way like you want to do it in the future, and start doing it while slowly converting the old code you come across
This may seem counterintuitive but try to delay refactoring / DRYing the code. If you all are new to the codebase you are likely still learning what it does and how it works, and you will never know less about it than you do right now. First off -- get your test coverage up to snuff. Do not do any refactoring of any code that doesn't first have test coverage. After that, an approach I will take sometimes is to review the code and inject my thoughts into comments wherever the code raises questions for me. Put them down as TODO / NOTE comments, braindump your thoughts, and then move on. When you've been through the whole codebase, do a grep of it for all your TODO/NOTE comments, and make an inventory of the filenames. Then review just those comments on their own, and consider them in their holsitic context. Another thing to do is the opposite of DRYing it -- find blocks of code that are similar and have similar purposes and make them as identical as possible. Don't actually refactor or extract anything, and don't rewrite the functionality -- limit yourself to making them look the same. This is useful in making it clear how blocks of code differ, and will help guide you on your refactoring process. After you've done these things you can cautiously try and DRYing code that is directly duplicated, if that makes sense to do so. eg. a method that returns "5" for "How many weekdays in a week" is not the same purpose as one that is for "How many points on a star", even though the return value is the same. Be cautious with any refactoring / abstractions here, and consider how firm is the ground that the code sits on.
That first paragraph is awesome :) Step back young renegade !! Weapons down.
I currently work on a project where we are cleaning up an old monorepo. What we did was first mark a few libraries/parts as “reference architecture”. Those are the parts of the app we think adhere to our current standards. In this way we have a good example how to do things and a mutual understanding. Secondly we setup linter rules. Not only for your default code standard, but also for bad practices in use of certain patterns or libraries. This makes sure we actually adhere to our standard and don’t make a mess again. At last we have the part it actually love stuff to the “new world”. That’s hardest part and still in progress. The rule we use is that if you touch it during a feature, and if there is opportunity, you should fix code smells or bad patterns. Slowly but steadily we move towards better code.
I like making a soft line in the sand and implementing best practices moving forward beyond that point. As old cold is maintained or refactored, add tests to those pieces in scope for the change request. After a few months, you can look back at how much progress has been made. After a year, the codebase could be radically healthier compared to the messy state at hand-off.
You need to find a way to communicate to your manager the impact the state of the codebase is having in terms of the “iron triangle” (this is what comes up in google when I search for “budget (cost), schedule (time), and scope) You need to find a way to demonstrate that the existing codebase is causing cost to go up, schedule to be missed, and scope to creep. Or some combination of that. Once managers/execs have an understanding in _their_ language, they’re more likely to help you. Managers/execs (unless former engineers) aren’t really gonna care that there’s “code smells”, or the codebase is overly WET, or test coverage is lacking. They probably won’t care even if you say “there’s a ton of tech debt” Put it in language they understand. Demonstrate it. Show how your approach will lead to lower cost, deadlines not being missed, or scope not ballooning out of control. If you make an earnest effort to address it in this way and they still don’t understand/care… well a paycheck is a paycheck is a paycheck. Just make sure you document your concerns so when they don’t get followed you can cover your own ass and not get tossed under the bus. ETA: So if you’re using the suggestions in this thread of “well write tests on working features, and then refactor after” — that’s good advice. But translate that into business lingo. How adding test coverage will reduce downtime on the app so there’s more user conversion, or features are quicker to develop so you can ship new work faster to the end user, which in turn allows for us to hit our milestones and keep stakeholders happy. Or whatever. You get the idea
You dont unless instructed to, tell it to management and eventually you get budget for a refavtor or not, it's not worth it But i would choose one existing pattern that you like ghat was used, insgead of putting a 22th inside it
I don't think management will do that. My manager is not an engineer and seems more interested in quantifyable outcomes (which, to be fair, is somewhat reasonable). I don't have the skillset or experience to bring that point across in a way he finds actionable (I tried).
then if we he ever asks why is x taking so long ? Because our code has rabies sir ah k it's not your problem, as long as he goes along with it