Next week I'm going to pair with Sean Massa to talk about Code Reviews in Node. In lieu of this I thought maybe I should write down some of the things I look for in a code review. My perspective on this topic is definitely not unique, but after years of doing these I think I have some ideas about what works and especially what doesn't! My first inclination was to dive in and organize anything that comes to mind into tips or patterns. Before I do that though, first I wanted to ask myself: "Why do code reviews?"
Certainly there are derivative reasons, but I find myself coming back to these two:
I'm sure there are other possible reasons, but this should be enough to get started. To avoid duplication, I found these fantastic posts that justify why these are important:
Misunderstandings and miscommunication are a leading cause of problems in software development. Especially with the prevalence of the mediating involvement anti-pattern. Writing code is a translation of the original idea and as with any translation, runs the risk of "lost in translation."
If you're a part of the team, revisit the topic to make sure you validate your assumptions. If you're not part of the team, invest some time in understanding the purpose before doing too much code review.
Once you know the requirements, watch for discrepancies.
Once we know what it should do, we can investigate how the author verified it.
Does our testing give everyone on the team, non technical people included, confidence that we are done? If not, how could it? Where would this be most beneficial?
The tests should be reviewed like any other code!
If the code is difficult to understand now, imagine how difficult it will be in a few months when anyone, the author included, is re-visiting it.
Here are some simple refactorings that can be done:
Complexity happens naturally, simplicity takes work and creativity. A second opinion can easily identify simpler alternatives.
There's only so much time in the day. What little time you have for code reviews, make sure it's well spent. This requires prioritizing what you review. As I've said before, ask "Does it really matter?". Focus on complexity, the business domain, challenging problems etc. Don't worry about administrative CRUD interfaces. Focus on areas you routinely have problems. Focus on what keeps you up at night!
Keep track of common problems, instead of relying on the review to fix them, focus instead on using reviews to identify them. Then, work as a team to address the problems. Some examples:
Using code reviews to catch repetitive issues is going to be a problem when the code review misses it or fails to happen.
One overlooked area of feedback is the commit log. Look for problems like
This normally happens as a consequence of doing code reviews. However, if we are cognizant of it we can maximize the benefits and actually avoid doing things that can inhibit learning.
Do the code review with the author involved, there's no faster way to learn than having both people sit down together. Obviously this can't always happen, but for significant functionality it's probably a wise idea.
There's a trade off between wasted time reviewing and not enough, there's no guarantee every piece of code will be reviewed. Instead of relying on a review to catch mistakes, try to minimize the mistakes that you think are likely so the review instead focuses on highly valuable topics like simplicity, readability, verification and other areas where you can learn from others. Do the very best work you are capable of so you can learn new ideas from the reviews.
Code reviews should be a process of feedback and discussion. Obviously there are times when you're refactoring and you just have to do it yourself. If you are intentionally doing a code review, try to involve the author in the changes.
In my experience, when I make the changes myself and ask the person to review them, I see more repeat behavior of the same problems, not a reduction. As a reviewer, be very cognizant of this risk.
The longer you wait, the more likely things that are important to discuss will be forgotten and the more likely the code will be in production causing problems anyways. Quick feedback is always beneficial. The longer you wait, the less beneficial a code review will be.
Pair programming is like the best form of code review, it happens instantaneously. Of course, make sure you use this judiciously. For example, there's probably no reason to use it on an administrative CRUD interface.
When people repeatedly make negligent changes and just don't care about the quality of the code, the code review won't help. I've made the mistake of trying to cleanup after this behavior and it never helps.
There's no way code reviews will solve this problem, they can only identify when it may be happening. Don't try to be the hero that always reviews this person's code, you will be very unhappy. Perhaps do some pair programming with the person to see if it's negligent or accidental, and then deal with it outside the code review process.
Unless you are dealing with someone who doesn't give a ..., assume the person does until proven otherwise. This will lead to a much more productive learning experience.
Sometimes you see something you don't understand, instead of inferring it's not necessary or was a "bad decision." Ask why it's there.
We all take pride in what we do, keep it constructive. Likewise, if someone is coming across offensively, point it out politely. It's tempting when you get repeat behavior or rudimentary problems to become frustrated, but remember these can be a sign of a problem in the process, it's often a sign that people are rushed. Sometimes code reviews cannot fix the underlying problem.
At the end of the day, a code review is an opportunity for feedback. We do them to improve quality and to learn. The specific techniques will be subjective but keep the end goal in mind. Leverage techniques of testing, refactoring, automation, design and every other aspect of software development in the process.
The first few discussions with a new consultant.
Ease into starting a project with new consultants. First invest in building some trust.
The first discussion is a great opportunity to talk about your business with the consultant. This understanding is invaluable and should be easy to talk about without advanced preparation. In subsequent discussions gradually introduce goals and problems.
Shared understanding is a great way to establish trust. Look for ways a consultant offers value through past experience, advice, and other feedback. This initial value can go a long way towards establishing trust.
Not to mention what it means for a consultant to invest the time to learn about your business and showing interest in what you do. Holding several discussions gives everyone a chance to reflect and bring even more to the table in subsequent discussions. This can be a quick way to weed out consultants that don't have the time to deliver quality solutions.
This is also an opportunity to show the consultant that you are willing to invest the time to build a quality solution.
Wouldn't your rather learn a bit about each other to see if the relationship will be a good fit?
The first few projects with a new consultant.
While taking on the risk of working with a new consultant, minimize other risks including time, complexity and low value. Nothing builds trust like a completed project that meets your objectives!
Mediating involvement happens when information flows from person A to B to C, where A and C rarely talk directly. A & C could be individuals or a group. In some cases there are multiple Bs!
Think about the telephone game where everyone sits in a circle passing a message around until it comes back to the source. It's often crazy what gets back, even with the best of intentions. Imagine having a conversation back and forth between the first and last person in the circle!
Full disclosure: I've been guilty of being a mediator, it's not about blame, it's about what we can do to avoid these issues:
In my experience:
This is very simple to mitigate. When a situation arises where involvement is needed, directly involve the responsible people in a conversation. This can't always happen immediately so keep track of these situations in an area visible to everyone so they can be addressed as soon as possible.
It's very common to hear from customers, "What will it cost to build X?" Naturally, I want to understand what X will accomplish. I consider this process understanding the value Y, the desired outcomes. Sometimes customers just demand a cost, but for those that are patient in analyzing the value these are the benefits they will reap:
My expertise is custom software development. If I don't know about Y I can't leverage my expertise to propose and build optimal solutions to achieve Y. I also don't have the opportunity to point out deficiencies in X. Imagine someone telling their doctor what medication to prescribe.
If you ask for results, you will likely get them. If you ask for features, there's no guarantee of results.
If I don't understand Y, when X doesn't accomplish it we'll have to try something else. You would be surprised, but when this happens I usually get requests for changes and new features, again with no guarantee of achieving Y. This has happened on every project I've worked on when Y wasn't discussed. Not only does this waste my time, it wastes yours. We are both stuck with the conundrum of who should pay for this wasted time.
Software development is an involved process, it's not just my time. Success requires your involvement in planning, implementation, user feedback, stakeholder feedback, testing, training and many other aspects. It's easy to overlook/underestimate this time commitment. Either way we both lose if we build failed solutions.
If we reduce wasted time, you will get results faster.
Less wasted time means less cost. Identifying and steering clear of low value projects means more of your money can go to high value projects.
Complexity in software happens naturally, simplicity takes expertise. Simplicity brings a whole host of benefits. If the focus is on features, features (complexity) will be maximized.
If the focus is on value, then value will be maximized! Low value features will take a back seat to higher value features.
In my experience, maintenance cost is exponentially proportional to features. Minimal features means minimal maintenance.
If your business needs to make deliveries you could buy a Lamborghini. However, the cost to maintain it will be much higher than many other equally viable choices.
The more features the more difficult it will be to introduce and/or change features in the future. For every feature we must be careful not to "break" it with any updates. Naturally, if we don't minimize features, over time it will become very difficult to introduce change. Ironically, I find the features most likely to get in the way are the ones that aren't even used!
Do you want to pay to maintain features you don't use?
If I know Y, when your business changes I can make recommendations about which Xs can be removed. If not, in my experience, they never get removed.
Here's a great post on why less code is better.
The more features, the harder it is for users to learn to use your software. Keeping things simple for users means less training. Training cost is proportional, probably exponentially so, to features.
If we both know the desired outcomes we can devise simple methods to monitor the effectiveness of the resulting features at achieving the outcomes. When features prove ineffective we can alter/remove them and try other strategies.
If we discuss value we can compare it to cost and make sure we're both making a wise investment decision. Without this analysis it's very easy to propose features that can lead to a marginal or negative ROI. In business it takes extreme discipline to make an investment decision. All too often I see everyone wrapped up in features and value takes a back seat. If we make this our focus, it's less likely to happen and you are more likely to see a significant ROI.
This analysis will naturally take longer the first few times it's done. Over time everyone will become more effective and efficient and the time will be significantly reduced. The capacity to make wise investment decisions in software will be magnified!
There's ample opportunity for any company to invest in high value projects. Low value work competes for limited resources to develop high value projects. Low value work is unlikely to make you happy, at least not compared to high value work. If you demonstrate an interest in maximizing value, it means we're much more likely to have a successful business relationship and we're both much more likely to prosper. Because of this I provide this analysis process for free. When we're done you will still make the decision if we move forward. That means before I charge a penny you will already be guaranteed to get something of value!
When discussing value we're likely to stumble upon roles that usually aren't involved until much later, if at all. These roles often provide perspectives that can avoid sub optimal solutions before we even start. One of these roles is the buyer, the person who sets the final expectations of outcomes. Imagine how difficult it is to achieve an expectation for someone you've never met! Also, users of the resulting software are often left out. For example, if we're trying to save time or expand capacity, doesn't it seem like a good idea to involve the person that does the work now?
It's very easy to identify missing roles when we discuss value and run into questions that can't be answered.
All of these benefits can be had by simply investing in an analysis of value. Once you get good at this it's often only a few hours investment per project. Is it worth the risk to skip this step?
