Just having someone look at your code is shockingly better than having no one ever look at it. If you start to do code reviews, you should be proud of yourselves. Just know, there’s a lot of ways to do code reviews wrong. There’s ways to waste time talking about things that don’t matter. There’s ways to upset people. There’s ways to miss code that doesn’t even compile. Let’s talk about it.
Review Tone
Make it about the code
It’s important to remember that you are reviewing a teammate’s code. Your goal should not only be to make the code they wrote better, but to make the teammate better. This even goes beyond just introducing them to new concepts, too. If getting their code reviewed is a positive experience, they’ll be more willing to write code for your team. If you are harsh, rude and nit picky, you might discourage them from working as hard.
Whenever you write a comment, critique the code, not the coder.
You forgot to normalize this value.
While this might seem small, to some people this is a devastating, savage insult. They were already nervous about doing a code review in the first place and now they forgot something. Again, to some people, this seems trivial and that the onerous should be on them to tough it out, but you have a choice. You can work with your less confident teammates or you can tear them down.
Allow yourself to be wrong
Removing the “you” from the previous comment, we get this:
This needs to be normalized.
This is better, but it still has a problem. You could absolutely be wrong about that. It’s better to come at this with a bit of humility:
I think this needs to be normalized.
Even if you are 110% sure that it should be normalized, it opens the other person up to judge if you’re correct, rather than to take your word as gospel. While this may be less important with people you are comfortable with, it matters a ton when you have any sort of seniority, authority or experience over the other person (Producer vs Programmer, Manager vs Employee, Graphics Programmer vs Tools Programmer). You can be wrong about things. You can misunderstand their code and offer a misguided suggestion. Leave the door open for discussion.
Avoid Animosity and Sarcasm
Like I said before, I think this should be normalized.
Again, I think this should be normalized.
Aaaaaand yep, this one should probably be normalized, too.
It can be frustrating when you tell people something and they end up not doing it. You tell them to not drive into a building and, oh look, my Jetta is in the middle of Target. Even if you told people in advance and it turns out you were right, you have to be humble. Nothing shuts people off more than “I told you so” comments. It’s arrogant and unnecessary.
It’s also partially your own fault. Why didn’t they listen to you? Did you say it in a way that made them less receptive to taking your suggestion? Did you say it at a poor moment so they weren’t focused on what you were saying? Did you say it too late for it to have an effect? Did you mix it in with a billion other things and it got lost in the conversation? Did you not write it down or set a reminder for it? Should this be something that’s handled by a tool? Is this something you should have been handling anyways?
You are on the same team. If you bring anger or frustration into code reviews, you are just making your own teammates worse. It’s like throwing a hammer out the window because someone dropped it on your toe.
Wikipedia has a great motto: Assume good faith. They aren’t making mistakes to mess with you. Always assume actions are caused by tiredness, frustration, accidents or just not knowing better. When you assume bad faith, you just raise tension on the team.
What you should be reviewing
Ensure the code is necessary
It’s really common that you have written an API or helper somewhere else that does exactly what they have just written. You might have hidden it in away in a private function of your class, but extracting that into a helper class and using it in both places ensures that an optimization to that function is applied to everything that does it.
Our codebase had an entire project dedicated to files with common helper functions in them. One was dedicated to functions that dealt specifically with files. Another had to do with random number generation. Another was a collection tweening function. Point is, everything could access it.
Add a file that has easing functions. Right now. It makes polishing your game so much easier. Also, add an overshoot ease function.
float Overshoot(float t) {
float overshoot = 1.70158f;
t -= 1;
return t * t * ((overshoot + 1) * t + overshoot) + 1;
}
Ensure the code is readable
If you are confused by something, always mention it. Don’t assume you’re stupid or that someone with more skill would understand it. The most likely scenario is that there needs to be a refactor or comment added.
Again, the most valuable resource on any team is engineer time. If the code that someone has written is unreadable, it is likely to waste someone else’s time trying to debug it. For the sake of argument, let’s assume the code is actually completely free of bugs. How do you know that when you’re debugging your own code when it goes through it? You’re now forced to understand what it’s doing when it goes through that code.
By understanding the code yourself, you’re increasing the bus factor of your team. When someone writes code, it has a bus factor of one. If they die, no one understands how it works until they read and understand it. By taking the time to actually understand what they’re doing, you increase the bus factor to 2. You can even start to find optimizations in their code now that it makes sense!
To focus on readability, you should be looking at grouping, ordering, comments, naming, variable usage, helper function usage and overall code complexity. CodeFactor.io looks at cyclomatic complexity of your code. In short, how many different ways can you go through this code if each conditional has a different truth value? If you have a lot of nested if statements with multiple conditionals and crazy for loops, it’s much harder to reason if the code actually works, even with a lot of tests. Try to encourage people to split functions up as much as possible.
Ok. I know what some of you are thinking.
“I saw some guy in some video say that splitting functions into helpers is actually bad style and inefficient.” ~Someone who has been led astray.
Let’s talk about that.
Ensure the code is as efficient as necessary
Any time you write code, you have to judge the readability and efficiency of your solution. Ask yourself this question:
“If I don’t do this optimization, what effect will this have on the user?”
If you have not measured the speed of your code in a hermetic test, you cannot talk about reducing readability for the sake of performance.
This goes for both sides! Give their code a test. Is it actually slow? Visual Studio has a CPU profiler to help identify where the bottlenecks are in your code.
In general, pick the simplest, most readable and most modular solution that doesn’t affect the end user’s experience negatively. You really don’t have to worry about function/object overhead or bit twiddling except in very unique scenarios. If you can prove that’s the scenario you are in, great. If you can’t, write engineer friendly code.
Ensure the code is correct
This is something robots struggle with. Look for bugs. Find ways to break the code. Make sure it’s cleaning up its resources. Make sure the tests actually work and that they get run.
Try tracing the code. Can you find ways to break it? What if you hand it garbage values? Thinking like a hacker. If you can break the function, it should be noted in the contract. If it isn’t noted, the function should be invulnerable.
Ensure the code is modular and extensible as necessary
People don’t like to have extra work handed to them, especially when they though they were done. Well, it turns out that data driving is good for just about everything.
Our game had a weapon system. It wasn’t data driven at first and it was kind of a nightmare to work with. When we refactored it, data driving made it an absolute joy to work with. We could start to hot load weapon parameters. We even made an editor for our weapons.
This came up in a lot of code reviews. I made a component that cycles through different stamps (think models, but with voxels) and put them into the world. I got the comment that it should be data driven. While it was strange at first, it was incredibly powerful to be able to simply change a text file to have a different ordering to the stamps! Our main menu could be whatever we wanted, and even get updated by other levels if we wanted!
We did this with the hub world as well. It was fed a list of levels and created portals for each. If we wanted to cut a level, it was as simple as changing one line in a text file, rather than recompiling our project. Look for any excuse to data drive!
What you should not be reviewing
Style
Very simply, you should have a linter. As mentioned here, you can use clang-format and set it up in about 20 minutes or less. If a mistake is trivial to fix, have robots do it. If a linter doesn’t exist that does what you need it to do, you can even create your own.
Note that these tools were created before we knew of the magic that is clang-format. All of these can be handled by clang-format automatically!
All projects in Visual Studio have Pre-Build, Pre-Link, and Post-Build events. Our Pre-Build events ran an executable that went through our directories to remove the new line at the end of files, convert all tabs into spaces and reorder our includes to be alphabetical. You can do a lot of checks like this! Whitespace at the end of lines can cause people to add trivial changes to unrelated pull requests. How trivial would it be to write a program that removes trailing spaces from every line?
This isn’t expensive either. How many files do you have in your code base that are internal? As long as you keep external and internal files separate, you can easily parse through everything in well under a second.
Now that we know how to review code, it’s time to learn how to have our own code reviewed.