We're a team of 7 building an enterprise application with Angular and Angular Material. We have different plans and features that can be toggled for each plan using a set of feature flags. We built granular controls such that, if required the user can be allowed to view a feature but not to interact with or change it. Everything worked great and we've built around 60 such feature flags.
We introduced a new feature that comes inside a set of tabs we already have as basic features. This new feature is supposed to be visible as an extra tab in the middle of other basic tabs.
The above stackblitz is a stripped down version of what we have in our codebase. It worked well in our testing and we were ready to ship it to staging that day and to production later that week. But we never checked if the contents of the individual tabs were rendering if the feature tab is turned off. The tab labels were in place and it felt enough to move forward as other tabs were untouched. In the final rounds of testing, one of our team members noticed a weird issue. Some of the tabs were not rendering the contents inside them.
It was a Friday evening and almost everyone has already shifted into the weekend mindset. This got everyone on their heels and we started debugging it. There were few API timeout issues in the console but that can't be breaking the rendering. We handled API errors gracefully for all the known cases. Any unknown errors will be caught and will be turned into something human readable. So it has to be something else.
We were trying to optimize the initial loading time and one of the change is to delay the off-screen components rendering until they have opened. It's an experiment that's happening in it's own branch. By any chance, that code got into development branch? I checked the commit history and ruled this out.
Luckily the same dev who discovered the issue also figured out why this is happening. We used to use bootstrap with angular and later migrated our codebase to Angular material. Our bootstrap implementation of tabs is just divs toggled with conditions based on the current selection. The conditions that hide and show the divs in bootstrap were copied when we migrated to Angular material. The divs were using a hidden attribute that checks the active tab. When we added the optional tab in the config object and toggled it, the feature tab is removed from the DOM but the config object still maintained the same tab order and count. So when we open each tab the inner condition was always false hiding the contents of the tab (Check app component and its template in the below example).
Once we found the issue fixing it was a debate. When asked no one had a clue about why the condition and config are there. The code is sitting there for a while and it bothered no one. The existence of a code block implies that it is tested and it is important. Everyone knows that removing those conditions is the correct solution. But we also kept asking what if we added that to solve something else? What if the condition is removed and the tabs fail in some other edge case? As a fix, we couldn't immediately remove the tab object and the hidden attribute from the inner content div. Instead, we rewrote the tab object by taking the optional feature tab into account.
This is a great example of the sunk-cost fallacy. We are maintaining code just because it is already there. We were forced to write new features to work around the old implementations. No one is ready to let go even if we know that's the right choice.
My takeaway from this is that it is harder to remove code than to add new code. A development team should be really careful about the new code they're adding and vigilant about removing any old code that's unnecessary really soon. Removing old code may not be easy as it sounds even from a well-written codebase. I'm actually a little late to the party, there are few great resources out there explaining how problematic old code can be such as @swyx's tweet here.
Gresham's Law applied to code:
Hard-to-Delete code drives out Easy-to-Delete code over time.
"Technical Debt" is the wrong metaphor. When we are so scared of our own codebase that we put on bandage after bandage, we should worry about "Technical Mummification" instead.11:31 AM - 24 Jul 2020
46
228
I found this Programming is terrible article that talks great length about writing code that is easy to delete. In my experience, Code that is easy to delete is mostly well written and structured code. I hope this article and the linked resources provoke some thoughts on your mind. Do you have any similar stories? Share with me in the comments.
Note: To handle the legacy implementation around the tabs, I created a new task in Jira and now testing it after rewriting it. So it is getting removed after all, but with thorough testing.
Top comments (4)
Vignesh.. Thanks for this post. Once code is released the issue of backwards compatibility is a major concern. I think your team did the right thing. Now that its a known potential bug. It can be worked on later. It does however underscore the importance of regression testing.
I've recently converted to Cypress for regression testing. It makes me feel safe..
Good luck...
Thanks for the suggestion John, I checked Cypress before and couldn't use it because it was not supported by browserstack. We are using browserstack to run E2E tests using protractor before deploying to staging. The main issue here is, Our app is constantly evolving and it is really hard to keep the tests in the same pace. we are trying to do that and improve coverage.
Yes understood. Are you using page object models? And can you auto create them?
No, I heard about them when reading about selenium, but I am not sure of them in Angular and protractor's context. But when I think about it for E2E tests, it shouldn't matter if I use angular or just Vanilla JS. So I will look into it and see if I can bring them into our tests.