Recently I started doing something I’ve always wanted to do but never actually got into: code reviews. I have some prior experience with code but not enough to write on an enterprise level. I always feared that my lack of ‘coding standards’ would be a big barrier to break. However, since reviewing code for a month or two it is going better than expected.
This is a story of how I found a new way of providing value.
During this time I mostly learned by looking at the code reviews other team-members were making. Seeing what kind of remarks they gave and applying that in other pull requests (PR) that I would review. In the end the PRs must be approved, which I don’t often do based on just a code review. Luckily the PRs are deployable themselves, and thus testable.
Most code isn’t maintained by just one person, so one important characteristic of code is that it must be readable. This means that if a more junior person can’t tell what a piece of code does, it must be improved.
An example of this is when the code I encountered set authorization levels. They set the levels using the number 1 to 3. When you know nothing about this code, you have no clue what authorization level 2 means. Instead it was improved to an enum value (list which represent other values). We used AUTH.low, AUTH.high and AUTH.impersonated which made the whole part instantly more readable.
Readability is also important for the naming of variables and functions so that when you read the name, you know exactly what you’re dealing with. Whenever I’m in doubt I put a comment asking to explain what it does. This often opens up the discussion, and either I understand it better, or it gets renamed. Because what does the ‘DisableRemoveLoginFilter’ do anyway? Does it disable the filter? Does it do the opposite?
Another thing that I often notice in code is a conditional statement where not all cases are covered. An IF-statement that has no ELSE-part might be incomplete or a potential bug waiting to happen. This is also the case for a SWITCH-statement that has no default case. This is something I will often remark and it is either explained to me what happens, or improved. These default cases will cover all cases that are not taken up and even catch things like ‘null’ or NaN values. We had a switch where people would be ‘success’ or ‘fail’. When neither applied, like the newly added ‘impersonated’, it might have failed if we forgot to update it. That’s where I requested to add a ‘default’ option, so we would be safe if something were to go wrong.
Not only can you add comments or ask for explanations, you can also contribute to the code yourself. When code is checked in, the unit-tests for that code are also checked in. Not only that but they can also be improved. In my case, I saw that they checked nothing but the happy path. Instead of just commenting, I asked the developer to help me check-out the code and added the tests myself. This was then added to the PR which is now easily improved, and next time, I can just contribute my own. Doing this helps you understand what your services do better and potentially even prevent a bug!
Working together with developers will also improve the relationship. It proves that testers are not just there to find bugs. Doing this shows you care for the work they did. You even begin speaking ‘their’ language.
When you get involved in code and the quality of it, you bring the topic of quality to a level where developers are directly involved. Quality doesn’t start with testing, it starts with a mindset and is reflected in good code. These are places I believe you as a tester should also be active, helping to improve the quality together with the team.