So I recently came across a huge React codebase that was hard to understand. There was no proper documentation, it wasn't clear how data is passed around and there are no automated unit tests. Now this is just the tip of the iceberg and I don't want to waste your time with a long rant. I would rather spend time discussing and coming up with solutions.
I think people write bad code (myself included) not because they are bad people but because it is easy to do so, unrealistic deadlines make it even worse. I think if it were hard to write bad code, there would be less of it around.
It is also not just a technical problem, people have to enforce it too. Have a well defined spec on how your codebase should be structured, documented and tested. Then don't approve that PR unless it passes those specs.
I could go on about this, I want to know how you feel on this topic and how you go about fixing it.
Top comments (7)
I feel your pain and frustration, I've been there as well...
I guess some of those issues can be contained and managed with some automation such as linters that set some basic quality standards such as correct formatting, spelling as well as requiring code documentation for exported functions and components.
Linters & Code Complexity Analysis
When a commit is pushed or a PR is submitted then those linters should just run and reject or annotate a contribution that violates those rules, and make the reviewers of the PR aware of that.
Now even with properly formatted and documented code it's still possible to write bad code that is hard to read and maintain. There are some linters out there that perform code complexity analysis and measure "cognitive load". That can be used for preventing deeply nested control flow structures or overly long functions or methods.
Those complexity issues can be usually resolved through refactoring and breaking those big units into smaller units that are often easier to read and verify through unit tests.
Static Code Analysis
Another type of tool is static code analysis that tries to find common types of bugs (e.g. null pointer exceptions / undefined errors). Those analysis tools can be also added to the code review workflow, build process and / or as a git commit hook.
Using TypeScript or Flow
I made the full migration to JS + Flow in a larger React project I'm currently managing.
Proper type hinting is valuable because it adds context to your code that aids your IDE for example with providing you with much better code completion. That speeds up dev time and prevents you from making simple mistakes.
Flow for example also allows you to declare your own data types. You can use that for mirroring API data types, and your IDE provides you with code completion for those object fields.
Type hints add a lot of clarity to the code base:
Type hinting can be enforced and verified with linters btw, so also that can be automated to improve the quality of new contributions.
Team Discipline and Establishing New Standards
It's mostly up to team discipline and communicating to the higher ups that hard to understand, poorly tested, poorly documented code will cost them in the long run. The team needs to decide to tackle this and dedicate their time to source code quality control.
Doing Proper Code Review
Your team should also take the code review process seriously and try to discuss ways to improve the contribution in a constructive manner.
I've been part of code review processes were reviewers basically just go through the motions and give it the green light without even leaving a comment behind. I've been guilty of that, too. It's understandable because it takes time to do proper code review. But it's time well spent for the sake of the code base and overall health of the system the team is working on. It should be simply seen as part of the development job.
It's probably also a team / company culture problem. Reviews usually contain statements and corrections that can be taken personally by the contributor. And some reviewers might even make it personal. So it's important to turn the process into something friendly that is thorough nevertheless. There are numerous code review checklists on the Internet that can be taken as guidelines.
Unit Tests Document Expected Behavior
I think of unit tests less as a verification tool that the code base is bug free. That's pretty much impossible to know for sure. I see it more as a documentation for the expected behavior of the software. They should go through expected scenarios and their inputs, and verify that the expected output is produced by the tested code. Tests should have self-describing names that briefly summarize each scenario.
Your team could require that each contribution must come with unit tests. Especially for bug fixes, to make sure that the bug does not occur again.
I usually write my unit tests first to reproduce the bug on my local. Once that is taken care of, I fix the tested code until it passes my tests.
Testable Code Is Often Better Code
Writing unit tests has often a positive effect on the quality of the tested code. It nudges you towards writing code that is more easily testable in the first place. More easily testable code is also often more readable and easier to maintain.
Writing testable code typically requires you to decouple your business logic from framework dependencies that are usually complicated to set up in unit tests.
You would write dedicated functions that do one thing, and one thing only, which can be more easily tested. Decoupling your code from those dependencies also allows you to mock those dependencies more easily if they are needed for running your tests.
Slow but steady
Once better procedures are in place and enforce them through proper code review, and by using tools such as linters, code analysis, unit tests etc., you have at least some extra assurance that new changes are less likely to make things worse.
From there it would be basically a slow iterative process to get the rest of the code base up to speed, and I guess that would come naturally when new features need to be added to those older components, or bugs need to be fixed.
You've really hit the nail on the head. This is exactly what's needed. But as a junior dev coming into a large project with a lot of senior devs, it's a lot more intimidating to do so.
But in the other team where we all were in our first job, I laid out some of these requirements from the get go, and it made life a lot easier.
That's fantastic! How does it work for your new team day to day? For example, how do you handle code review? Were those requirements you laid out perceived as an obstacle or a chore, or were they well received by your team?
I just re-read this and I think you've done a great job explaining it all. It's too good to be sitting in a comment section, you could maybe post it.
I believe we all want to write good code and documentation. If we get the chance to start from scratch it usually starts with good intentions.
I think the main reason we at some point get sloppy and write bad code is because so much things are feature driven, so many corporates are feauture factorys and we get so less time and priorities.
Fix this, do that.. all at once, we needed this yesterday ..
There are only a few clean code companies that I know that sell good code as a feature.
Most in Management or Sales etc, do not view good code as a feature and for sure dont sell or advertise good and clean code to thier customers.. and honestly most customers don’t care either as they just dont know and do not ask for it..
That needs to change.
This is so true! I've done this myself as well.
I think they need to realise that good code will eventually lead to lesser bugs for the customers; devs and managers need to convey this to the sales team in words they can understand (no disrespect ofcourse)
This is one of the reasons I like the non-popular languages a lot more. For front-end development, I use Elm, which is a purely functional, statically typed programming language that forces you to write better code.
The backend equivalent of Elm would be Haskell.
Now they both have a pretty steep learning curve, especially if you're coming from OOP. But I think they are both worth learning.