Some thoughts on code review

Github has added some new features to pull requests that make for a richer code review cycle. To commemorate this, I thought I’d share some words about code review–not the Github stuff specifically (I’m still exploring!) but the process in general.

Code review is a cornerstone of good team-based software development. It isn’t the same as pair-programming, and it isn’t a replacement for it. By the same token, pair programming is not a replacement for code review. The two practices work in tandem–and, as I have suggested elsewhere, code review can also make solo coding a viable part of the development process. I consider it absolutely essential.

In this article I’m going to go over three guidelines for successful code reviewing:

  1. Do your code reviewing alone
  2. Be fair and flexible about style
  3. Treat unclarity as a red flag

I’ll say something about each of these guidelines in turn.

Do your code reviewing alone

If you are pairing on code review, your pair is either (a) one of the people who wrote the code, or (b) someone else who didn’t write the code. Both options are problematic.

Reviewing code with someone who originally worked on the code inevitably means that you’re seeing the code through their eyes. You’re really asking for a guided tour, not doing a review. In review, code should speak for itself. You don’t want someone there explaining it to you. That’s not representative of what it would be like to work on it yourself at a later date, or for someone new to the team to pick up a story that required them to work on this code.

Moreover, you might–without meaning to–put your pair on the defensive. “Why did you do this?” you might ask, pointing to some line or lines of code. The person who wrote or co-wrote those lines then has to mount a defense of them, and that can be frustrating as well as unproductive. Lines of code in isolation sometimes don’t explain themselves; you, as the reviewer, might have to look at the whole class or file to see the rationale for the code’s structure or naming conventions. But quizzing the coders on why they did something is not the same, cognitively, as looking through the code yourself and letting the code speak (or fail to speak) to you itself.

Doing code review in a pair where neither of you worked on the original code is also not advisable. Each of you will pick up on different things, and pointing those things out–not to mention scrolling up and down and flipping among files out of sync with each other–is simply not the same process or experience as examining the code yourself. Your different paths through both seeing and understanding the code will cause interference patterns and almost certainly make the task less efficient and less effective.

Now this doesn’t mean that you should never sit down with someone else and look at code! I’m talking specifically about code review: someone has just committed some code, and it needs that next pair of eyes. You may have occasion to discuss code you didn’t write with someone who did write it, and you may have occasion to discuss code with someone else when neither of you wrote it. (Think of a pair picking up a ticket on some code by people who have left the team.) What I’m saying is not that it’s bad for these things to happen. What I’m saying is that they are not code review.

Be fair and flexible about style

Your team should have a working agreement about code style. It may be a blanket agreement (“When we write Python, it has to pass Pylint”). Or it may be a very specific agreement (“No empty parentheses in Ruby method definitions”). How you determine your team or “house” style is a matter for the team and the organization.

When you do code review, you should not hesitate to point out violations of team style agreements. However, if the code you’re reviewing does not violate team style–but happens not to be in the style you yourself would have used–leave it alone. Code review isn’t about making everything look as if you had written it.

If you spot something stylistic that you think no one on the team should be doing, make a note of it and bring it up later as a possible addendum to your team’s style agreement. But don’t flag it in code review if it isn’t already part of that agreement. Code review is not the place for stylistic ping-pong matches.

Treat unclarity as a red flag

Perhaps the main thing you should be looking for during code review is code that is not clear. Unclarity can take many forms; but whatever form it takes, it should be treated as a show-stopper. Unclear code means someone is going to get confused at some point down the road–possibly including the people who wrote the code in the first place. (We’ve all had that happen!) You should immediately write a comment when you come across something unclear. In your comment you should include some analysis as to why you think it’s unclear: a method that’s doing too much and is hard to follow, confusing variable naming, and so forth.

This guideline is actually not only important in its own right, but also figures as another reason not to do code review alone. If you do code review in pairs, it’s too easy to explain the code to each other as you go. You don’t get enough of a chance to determine whether or not the code per se is clear.

Don’t be afraid of insulting someone’s code by saying it’s unclear. By the same token, if someone reviews your code and says they didn’t understand or couldn’t follow something–whether because of logic or naming or anything else–don’t defend or explain your code. It’s likely that you could explain the unclarity away. But you shouldn’t have to, and in the future when other people look at your code, you won’t have the opportunity to. Consider the finding of unclarity to be a reality check, and a very compelling indication that you need to change the code.

Happy reviewing!

Prostate Cancer Awareness begins with prostate awareness

This is not medical advice. Please consult your doctor(s) about medical questions and issues.

September is Prostate Cancer Awareness Month. I’m pleased to say that it’s also the month when my three-year post-surgery blood test returned good results. My PSA (more about PSA below) is still “undetectable,” as it has been since two months after my July, 2013 radical prostatectomy.

Prostate cancer awareness is very, very important. But what’s struck me forcefully over my three years as a prostate cancer patient and survivor is the number of men who have no idea what their prostate does. I’ve started asking them point-blank. Nine times out of ten, I get shrugs. Many men think the prostate does nothing, like the appendix. Those who know what the prostate does tend to have a medical background, or to have had prostate cancer themselves or in the family.

The prostate secretes semen. The testicles do not secrete semen, despite vulgarisms that suggest otherwise (like “bust a nut”). The testicles produce sperm cells. Those are the things that swim upstream looking for eggs to fertilize. Semen is the delivery mechanism for sperm cells. Semen comes from the prostate.

It follows, then, that after your prostate is removed, you can no longer ejaculate. When I mention this fact to other men, I often get asked “Why not?” or “Are you sure?” The answers to these questions are, respectively, “Because semen comes from the prostate” and “Yes, I am fucking sure.”

Here are some generalities, based on what I have learned as member of the Reluctant Brotherhood.

The nerves that enable erection run along the prostate. If those nerves are implicated in the cancer, they have to be removed. If they’re not cancerous, they still have to be peeled away from the prostate, which traumatizes them. In those cases they take a while to recover, possibly measured in years. In short, erectile function after a prostatectomy covers a wide spectrum. And medicine and/or devices play a role for many men.

While ejaculation is not possible without a prostate, orgasm is–even in the absence of erection. The strength and quality of orgasm will vary from man to man, and may improve over time.

Urinary incontinence after a prostatectomy also varies on a case-by-case basis (that’s a pattern with this disease and its treatments). At its worst, you’re looking at a lifetime of Depends and other ameliorative measures. But if you’re lucky, you don’t have any serious problems with incontinence. Maybe a little bit when you sneeze.

The main way you detect prostate cancer, or at least become suspicious of its presence, is through a PSA test. PSA stands for Prostate Specific Antigen. It shows up in the blood in higher concentrations when there’s cancer. If the doctor feels it’s warranted, they (yes, I know they is really plural but I hate “he or she”) will order a biopsy. A digital exam of the rectum is also used in diagnosing cancer and other prostate disorders.

Please do not fear any of these diagnostic techniques. Do them if your doctor suggests them. In fact, if you’re old enough–I think the guidelines say forty, but again, I’m not dispensing medical advice so please research it–and/or have a relevant family history, you should ask about them.

As regards treatment, the big three are surgery, radiation, and “active surveillance” (sometimes called “watchful waiting”). Active surveillance means not intervening, but continuing to measure and monitor via PSA and possibly follow-up biopsies, and then intervening with surgery or radiation if it becomes necessary. In my case, active surveillance was not an option because while the cancer was not terribly aggressive, there was quite a lot of it. I talked to two radiologists and two surgeons, and decided that surgery was right for me.

Prostate cancer is a grim disease. It has a reputation as a “good” cancer, because survival rates when it’s caught early are pretty spectacular and because some manifestations of it are slow-growing enough not to warrant intervention. But the permanent changes from treatment are significant in every case, and extremely significant in some cases. And around 30,000 men die from it every year in the United States. It’s not to be taken lightly.

So be prostate (cancer) aware, this month and every month.

Again: This is not medical advice. Please consult your doctor(s) about medical questions and issues.