6.14.2013

The value of code reviews

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?"

Why do code reviews?

Certainly there are derivative reasons, but I find myself coming back to these two:

  • Improve quality
  • Shared learning

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:

Improving Quality

Ask "What were the requirements?"

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.

Ask "How do we know this works?"

Once we know what it should do, we can investigate how the author verified it.

  • Manual Verification? - If the author used manual testing, it's possibly worth keeping track of that somewhere so the same manual tests can be repeated in the future. Study or inquire about these.
  • Automated Tests? - Review them to see if they match the requirements.
  • On faith? - Even if it's simple, it doesn't hurt to manually verify the happy paths. If it's complex, consider automation.

Ask "How can we improve testing?"

  • Are there missing scenarios?
  • Is there an elephant in the room after implementing a feature - things that haven't been discussed that could be a problem?
  • Is there a way easily automate testing?
  • What testing scenarios could be added?

Ask "Do the tests help us communicate that we are done?"

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?

Ask "Are tests explicit?"

  • Can you easily identify the AAAs (Arrange, Act, Assert)?
  • Are the tests difficult to understand?
  • Are the tests concise?

The tests should be reviewed like any other code!

Ask "Does the code make sense? Is it easy to follow?"

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:

  • Rename abbreviated terms
  • Reorder code
  • Replace comments with composition
  • Extract class from parameters

Ask "Is this code simple?"

Complexity happens naturally, simplicity takes work and creativity. A second opinion can easily identify simpler alternatives.

  • Prefer explicit over magic
  • Prefer less code
  • Prefer declarative code, separate what from how

Focus on what matters

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!

Identify repeat issues and do a root cause analysis

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:

  • Code formatting, style, and layout inconsistencies - perhaps invest in a tool to do this for you
  • Missing error handling - perhaps identify patterns or languages to get around these issues
  • Unused code - perhaps invest in a static analysis tool

Using code reviews to catch repetitive issues is going to be a problem when the code review misses it or fails to happen.

Ask "Does the commit log help organize the changes?"

One overlooked area of feedback is the commit log. Look for problems like

  • Files committed separately
  • Infrequent commits
  • Commits based on components of technology and not business value. IE: a commit for the controller, a commit for the model, a commit for the database changes and a commit for the view. Instead it's often much more useful to have these organized into one commit per use case/story/feature, especially when doing code reviews!

Shared Learning

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.

Review together

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.

Don't rely on reviews to catch your mistakes

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.

Avoid doing the work for the other person

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.

Review sooner rather than later

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

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.

Don't use reviews to fix problems from people who don't give a ...

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.

Assume good intentions

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.

Ask why if something doesn't make sense

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.

Avoid taking offense and try not to be offensive

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.

TLDR

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.

6.14.2013

Pattern: Establish Trust First

Applicability

The first few discussions with a new consultant.

Definition

Ease into starting a project with new consultants. First invest in building some trust.

Mechanics

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?

Tips

  • Make the first meeting only about your business.
  • Subsequent meetings can lead into objectives (goals and problems).
  • Assess the trust you have established, keep notes!
  • Avoid talking about solutions in these meetings. Solutions in software development often take significant time to develop. Work on establishing the trust necessary to justify investing time in devising solutions.
  • Avoid talking about costs in these meetings. Costs are unknown before a thorough analysis of solutions. Instead talk about pricing methodology to determine if it will fit into an investment model you are comfortable with.
6.13.2013

Pattern: Reduced Risk Initial Project

Applicability

The first few projects with a new consultant.

Definition

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!

Mechanics

  • Minimize Time - Work with the consultant to identify a project that can be completed within a time-line that is quick for your organization. I would recommended something around a month or less.
  • Avoid asking "Can X be done within a month?" Instead let the consultant identify the time-frames and compare relative time-frames to find the quickest projects.
  • Ensure that quick does not mean rushed. Quick means there isn't a large time investment. Rushed means efforts will be doubled up which actually increases risk.
  • Stick to a simple objective with clearly defined value and simple measures to gauge progress. This will lead to a much more definitive end of the project.
  • Maximize Value - Make sure the project is of high value. It's exponentially more difficult to build trust if there's not much value to be gained from the relationship. High value leads to better commitment on both sides. Honestly, I never recommend low value projects, I've never found a company short on high value projects but I've seen plenty of low value projects get in the way.
  • Avoid Complexity - If you have a set of projects that seem equal in value and time, pick the least complicated project. Complexity almost always leads to unknown risks.
  • Be prepared to discuss several sets of objectives to see which project minimizes these risks the most.

Consequences

  • Complex, higher value projects will have to wait.
6.13.2013

Antipattern: Mediating Involvement

Definition

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!

Consequences

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:

  • Miscommunication / misunderstanding
    • Even in direct conversation this happens.
    • It's even more likely in complex matters like software development.
  • Lost in translation
    • The mediator is very likely to summarize perspectives. Important aspects can easily be lost.
    • What if there was a miscommunication / misunderstanding in conveying the original information to the mediator?
  • Lack of creativity
    • Less minds = less creativity
    • Can't draw on the experiences of those that aren't involved.
  • Assumptions waste time and resources
    • Sometimes I'll ask a follow up question and a mediator will give me an answer they think is accurate when it's not.
    • Only after the software is built do we find out otherwise... ouch!
  • Unrefined information
    • No ability to carry on a quick back and forth conversation to refine ideas.
  • Huge delays in feedback
    • When it does happen, it often takes days for SIMPLE questions!
  • Follow up fails to happen
    • Follow up always runs this risk, even the most organized of people make mistakes.
    • I've never met a mediator who hasn't failed to follow up multiple times, even on critical issues!
    • Sometimes the mediator doesn't understand the value of following up, so there's no impetus to follow through.

In my experience:

  • Nearly every time someone speaks for someone else, I have a follow up question they can't answer.
  • Nearly every time I think I understood what someone said for someone else, eventually something comes up that was miscommunicated or misunderstood.
  • I've been blown away by the ideas that come from those that are often excluded.
  • The majority of times people say they will follow up I have to ask multiple times or just go to the source directly.
  • Countless times I've had to give up on mediators following up and make assumptions.
  • Most miscommunication / misunderstanding traces back to mediation, it's rarely from direct communication.
  • Software development is complex and time consuming. If it's not worth the time to involve everyone in simple conversations it's not worth the time to be developing the software.

Mitigation

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.

  • Clearly define roles and responsibilities
    • Users, Buyer, Consultant, Stakeholders, Subcontractors etc
    • Introduce everyone as soon as possible
    • Exchange contact information (email, phone etc)
  • Foster a team approach.
  • Everyone should avoid speaking for anyone else.
  • Consultant's shouldn't mediate the involvement of employees and sub contractors.
  • Analysts / managers shouldn't mediate the buyer nor the users.
6.12.2013

Why analyzing value gets you results.

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:

Optimal solutions

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.

Results

If you ask for results, you will likely get them. If you ask for features, there's no guarantee of results.

Significantly less wasted time

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.

Timely results

If we reduce wasted time, you will get results faster.

Reduced cost

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.

Simplicity

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.

Maximum value

If the focus is on value, then value will be maximized! Low value features will take a back seat to higher value features.

Minimized maintenance

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.

Flexibility to change

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!

Reduced unused features

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.

Reduced training cost

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.

Pivoting on effectiveness

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.

Disciplined investment analysis

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.

Increased investment capacity

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!

Free investment analysis

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!

Identify missing perspectives

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.

Reduced risk

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?