How to: Code Reviews

I did a presentation on Code Reviews to the rest of our development team this morning, so I reckoned I might as well share the contents here :)

So, what's a code review?

Here's wikipedia's definition:

A code review is systematic examination (often as peer review) of computer
source code intended to find and fix mistakes overlooked in the initial
development phase, improving overall quality of software and can also be used as
a tool to better develop skills at the same time.

I actually think that's an awesome definition, but rather long winded. Here's the 'Jo Translation':

Somebody looking at your code and finding your stuff-ups – you might even learn
something!

There's a few other things that stand out in my mind as defining code reviews:

• They serve as a 'consistency check' over a project - making sure we're all doing things the same way, and following what a colleague of mine likes to call the 'design vision'.


• They're one way to make sure that someone else has a vague idea of how your code works - making sure that the project will go on even you get hit by a bus tomorrow morning.


• They're a chance to raise any issues or concerns you have - one-on-one time with your team lead, make the most of it! "I'm not quite sure how to approach this...", "Is there a better way to do this..." or even "I'm feeling really stressed right now"


• They're a learning opportunity, not just for the reviewee, but the reviewer as well. There have been a number of occasions when I've been reviewing someone else's code and have really liked the approach they've taken to accomplish something complex.

How do you do a code review?

If you can't review everything, start by figuring out what is most important to review - functionality which is critical to the system or code which is particularly complex. Once you've figured that out, sit down with the reviewee and get them to walk you through (and talk through) the code in question.

I think the easiest way to do this is: get them to close all open files in the IDE, collapse all projects in the solution view and then go through things one at a time. I know it seems petty, but it keeps things really clear and forces you to go through things sequentially and not jump around. Sometimes the very act of creating a 'blank canvas' like that puts the person into 'review mode' and can help prevent things degenerating into a simple chat about where things are at.

I'll usually try to track through a particular piece of functionality, which means that reviews tend to start at the UI and track back through the layers. "The user clicks this button, which fires this event, which is handled by this, which calls this, which goes to this web service..."

I can't over-emphasise the importance of talking through code at this level. Have you heard of the 'Teddy Bear effect'? Apparently the name comes from a university who once put a teddy bear near the help desk in the computer science labs. Students had to explain their problem to the teddy before they could ask the tutors about it. Why? Because the teddy often solved their problem.

I remember one particular student from my time as a lab tutor who would frequently come up and tell me at length about some issue he was having, only to finish with "Oh, yeah. That'll work. Thanks!" All without me saying a word...

There's something about putting the inner workings of code into words that forces us to really think through what it's actually doing, as opposed to what we think it's doing.

Anyway, back from the tangent. When reviewing code, start at a high(ish) level of design (i.e. method level) and look for things like:

• Any un-necessary duplicated code, indicating something that could be abstracted out


• Error handling


• Glaring security holes


• Unit tests - not just that they exist but that they actually cover something of value

Ask questions: Can that ever be null? What if the web service is down? Does it handle leap years? What if...

Then check for lower level bits and pieces:

• Adherence to coding standards


• Readability


• Consistency


• Commenting


• Hard coding (there shouldn't be any!)


• Sensible' code - i.e. nothing that could end up on The Daily WTF

You can also check things like logging, auditing, concurrency, performance, data sensitivity, caching, you name it. If you can think of it, and it's important to your project (or is your pet peeve) and is checkable quickly: check it. Why not?


Do code reviews early and often. They're especially important at the beginning of a project (or with someone new to the team) when you're still establishing your 'vision'/approach/whatever. Also, make sure they're done before you release anything! There are two ways to decide when to do them:

• Schedule a review regularly - this is what I do. I use weekly for most people, fortnightly later in the project and twice weekly for new grads or anyone I'm concerned about.


• Review whenever someone tells you that a task is 'done'. I haven't tried this myself yet, but an experienced colleague recommended it and I like the idea. Might try this on a future project :)

Make sure that your code review time gets included in the budget right from the start. Half an hour per person per week is probably fair - it will be more than that at the start but less at the end of the project so should average out nicely.

Why bother with code reviews?

Hopefully by now this is obvious... but just to recap:

• We're all human, we all miss things and make mistakes. A code review can pick up some of those before they hit anyone else.


• Code reviews help solve the 'our critical person got hit by a bus' problem because someone else has an idea of how your code works.


• They help ensure consistency over a project, which helps make maintenance easier, which drops the cost of maintaining software, which makes your clients happy.


• It's a great learning opportunity for both reviewer and reviewee.


• When they're done right, code reviews improve software quality enormously.


• A scheduled code review provides and opportunity to discuss any issues or concerns.

That last point has been really important to me lately - trying to lead a team of ten developers it has been hard to keep on top of where everyone is at. Scheduled weekly code reviews gave me some quasi-uninteruptible individual time with everyone to catch up and make sure everything is going smoothly and there are no road blocks I need to deal with. Since this project has been more than a little hectic it also gave me time to just sit with the team to soothe or encourage and keep things moving. Invaluable.

How do I get me one of those?

If you're a team lead and aren't doing code reviews right now - start.

If you're anybody and aren't getting your code reviewed right now - go bug someone. Your team lead is a good candidate if you have one, but if you don't, pretty much anyone will do. It doesn't have to be someone more senior than you, although more experience does help. The only real criteria for a reviewer are:

• Someone who is willing to give you advice - i.e. they're not too scared of you and aren't just going to say "That looks fine... You're so wonderful... Please promote me now..."


• Someone whose advice you will listen to - you need to respect them enough to take their advice seriously and be willing to change your code as a result.

And that's it... happy reviewing :)

Decide to act

I've spent the last couple of days at a first aid course (which I recommend by the way, everyone should know that stuff) and one of the things they emphasised was that the first thing that needs to happen in any emergency situation is for someone to recognise an emergency exists and then decide to act.

We talked a lot about this in psychology courses at university too - it's frighteningly common for someone to be in trouble and for all the people who could have helped to do nothing. We talk about 'safety in numbers' but the reality is exactly the opposite - the more bystanders there are, the less likely anyone is to do anything. After all, "someone else probably already has"...

Side note: If you're ever in a situation where you need help and everyone is standing around staring blankly, single someone out of the crowd: "You, in the blue jacket, go call an ambulance." Once they're singled out they're usually happy to help.

People also seem to have an incredible hang-up about 'interfering', especially if we have some impression that the problem is within someone else's family. The family violence issue we're talking a lot about in New Zealand at the moment is a good example of this.

But I'm not planning on writing a post about first aid or group behaviour, as interesting as they are. I've been thinking that the same type of behaviour seems to appear in other situations too - including at work. Sometimes it's all too easy to identify a problem, think to yourself "someone should do something about that" and then go get a coffee. It's easy to assume that, since the first requirement for a management position is mind-reading abilities, all managers will know immediately if something isn't happening as it should.

But maybe, just maybe, you are the someone who should do something. And maybe, just maybe, your manager can't read your mind and, because no-one else has talked to them either, doesn't know there's a problem. Is it possible?

Many people are, quite rightly, concerned about becoming the person who always whines about everything. Be assured that I'm not advocating that at all! People who complain about every little thing that it's possible to complain about drive me just as mad as I'm sure they drive you. Please don't do that :) What I am suggesting though is that, sometimes, there are genuine issues which need to be sorted out so that everyone can move forward and get the job done faster/better/more happily and those issues shouldn't be left for the magical 'someone' who will fix everything. You're someone aren't you? Why don't you do it?

Call me an idealist if you will, but here's how I see things: if you have a manager whose job description includes anything along the lines of 'team leadership' or 'ensuring efficiency and well-being of staff' then that person's job is to make your life easier. If you let them know what, in your work life, needs to made easier then you're helping them do their job. How nice of you :)

Again, don't bug them too much! Your manager is just as human as you are, which means they can get just as busy and stressed as you can. Don't just roll up to their desk and start your lecture - ask if you can make a time for a chat. Then you can grab a meeting room, close the door, and lecture to your heart's content for the half an hour they've set aside. It's not that difficult, and doesn't take much time, to respectfully and clearly state your concerns and finish with "I just thought you should be aware. Let me know there's anything you'd like me to do."

If you're still wondering whether to act or not, consider this: if you're having issues with something chances are that someone else is too. They're just assuming that someone else has already said something, that their manager has read their mind and that they shouldn't interfere. Do that person a favour and go sort things out for them.

Final note: remember that the step before 'decide to act' is to 'identify an emergency exists'. Make sure the issue you're having really is an issue and really is something that can be changed. Sometimes you just need to grit your teeth and get on with things, that's just life. If you're not sure which category your issue fits into, go talk to someone anyway and ask them whether they think it's worth doing something about.

Old cliched quote which is still true and rather appropriate:

God grant me the serenity to accept the things I cannot change, the courage
to change the things I can and the wisdom to know the difference.

Headless Chicken

I've been officially a Team Lead for a while now but this week it's really started to feel like it. Design was interesting, new to me and provided lots of scary opportunities to stand in front of clients and try to explain myself but it was nothing like this week. Firstly, I was then leading a team of... uh... me. Not so much leadership required, although more than you'd think! Secondly, because I hadn't managed to be involved in that stage of a project before, I was still learning lots and it felt more like a move sideways rather than up. Last week, however, (drum roll please) we started development.

I was really looking forward to this. This was the part of the project where I felt I best understood the team lead role and what I would be expected to do. This is the bit I love - looking at a blank screen and starting to throw stuff onto it, then watching things take shape. And, this time, I'd be running it. I'd get to play with the hard/funky bits, pass on to someone else what I hope I've learnt of how to code well and support them so they could code freely without being caught up with frustrations...

Summary of how it's gone: I'm exhausted.

Admittedly this is partly because there's been other stuff going on this past week which has resulted in some late nights... But I'm still surprised at just how tiring it's been, and how little I've actually got done. Somehow, I'm sure I'm busy all day but I get to the end and my to-do list is longer than when I started! Plus, I only spend about one hour per day working on a use case - if I'm lucky. It's a big shift in thinking and ways of looking at things - I'm finding it hard to plan or estimate anything with the constant interruptions / context shifts (emails, questions, meetings, phone calls...) proving a bit of a challenge. I miss being able to sit down for a couple of hours, fully focus on something and see it done.

Sorry, I didn't mean to complain. It's been fun too - I have got to do some of the background helper-type code I was looking forward to and it's been nice discovering I have some useful knowledge to pass on. I am hoping things will settle down a bit as the project settles in a bit more and there's not so much running around to do. I'm also understanding more and more why the team lead on my previous project starts work really early in the morning!

Anyway, what I've been musing about today is whether there's a better way I could be doing this so I look a little less like a headless chicken. I seem to spend a lot of time trying to figure out what I should currently be doing, or what I was doing before I was interrupted, or what someone else needs to do. I have to-do lists (usually more than one, perhaps that's the issue) and post-it notes and a project plan but things still keep slipping through. Then I end up merely reacting to things rather than being in control and that doesn't work. So... ideas anyone?

A manager mentioned last week that it's far more efficient to only check your email a few times a day rather than jumping on messages the moment they arrive. I'd heard the theory before, and it does make sense since you context-switch less often, but so many people send emails expecting instant answers that it's difficult to put into practice. Worth a try? Not sure. I'm certainly considering it as an option - would make for one less type of interruption at least.

I'm also wondering whether I'm trying to cover too much. Letting go of things a bit and delegating more is definitely on the cards for the weeks to come. I don't have a particularly good track record of letting go of anything but we'll see how we go. Just another needful skill to learn. I find it ironic that not so long ago I was bugging my team lead that he needed to learn to delegate...

Beyond that, I'm banking on my brain adjusting to keeping track of many more things at once!

Coffee

I had to take a workmate out for coffee on friday. While other workmates have previously taken me out for coffee when I needed it, this is the first time I've done it for someone else and it's made me rather sad.

To give you some perspective: the workmate in question is the team lead on the other half of a large project I'm working on. We don't work directly together but closely enough that I know what's going on. I've been making a point of keeping an eye on things for a while, going over to chat, asking him how things are coming along etc. On friday afternoon we had a team meeting in which we had, once again, gone over the various things that are making a particular section of the work less-than-fun. There had been some infrastructure issues between our site and the client site so progress had been slow and frustration levels were pretty high. Add to that another team member who was struggling and needed help, the senior team member busy with other projects and not able to give much time and a go-live date approaching fast... Result: one stressed and unhappy team lead (surprise surprise).

After the project manager had closed the meeting and left, a few of us just sat and looked at each other for a while until I informed my workmate that we should go get some coffee. We had coffee, and cakes, and I let him talk for a while about how upset he was. Then we talked about the shed he was going to be building over the weekend and his little son... and then went back to work. All well and good, and I was glad to be able to be there for him, but not how things should have been.

Now, some of the factors stressing him out could perhaps have been avoided, but I don't want to go into that here and I think most things are out of our direct control. Cause is beside the point, what I do think is a problem is that we had an upset staff member and it was left to me to do the coffee thing. It shouldn't have been me. I didn't have the authority to say what needed to be said (that he was doing a great job) or to offer help because I'm not involved enough or senior enough for it to mean anything.

Our project manager did, in fact, shout us all coffee on thursday. That was great and we all enjoyed a break and a chance to chat to everyone but there is a difference between shouting the team coffee and taking a particular staff member out to provide some one-on-one support/appreciation. Individual attention is incredibly important, especially when things aren't going so well. When managing a difficult project, I think it's crucial to ensure that each individual member of the team believes that you know and care how things are going for them. If you're pushed for time, at least make sure you have that kind of a relationship with the team lead and encourage them to do the same for the rest of the team. You don't need to take them out for coffee every time; just pull up a chair next to their desk and have a chat, then remember what issues they mentioned and ask them later how they're progressing.

I know I'm sounding very idealistic and that it's difficult to keep track of things to that degree but it is possible and I believe it's worth it. Go on, take someone who needs it out for coffee tomorrow :)

Tick

Had a conversation with a project manager recently in which she said that she tries to figure out what motivates each person on her teams and what relieves their stress. In short, what makes them tick. She says I was easy - it's all about the chocolate!

Admittedly, I do love my chocolate and I have been known to accept undesirable tasks on the condition someone buys me chocolate. But I think she's missed a couple of things which are pretty major. I function not solely on chocolate but also on feeling like I've accomplished something useful. This made up of 2 things:

1. Accomplishing something, anything! This means seeing progress, being able to cross something off the list, having some sense of achievement. We all need this, projects where progress seems to go backwards have been described by workmates as 'soul destroying'.
2. Other people (colleagues or clients) appreciating what I've done. Recognition matters lots to me: a piece of paper with 'well done' on it will often stay on my desk for weeks! This is the bit that transforms "I've done something" into "I've done something useful".

No amount of chocolate will turn around a project where developers' work seems to be leading nowhere and no-one is acknowledging the effort they put in. I was involved in a project that ended up like that last year and the turnover rate was unbelievably high. Chocolate does contribute towards the second one though: "Someone bought me chocolate therefore they must appreciate me somewhat".

The other thing that I've noticed about myself is that I'm an information junkie. I'll put up with pretty much anything (including a lack of chocolate!) if I have all the information around what, who, when and why. Especially why. On the other hand, if I know something's going on but I don't have the details I get very frustrated very fast. One of my workmates in particular tends to try and 'protect' me from some of the larger client issues we come up against, I don't think he realises I'd be less stressed being involved than wondering what everyone else is trying so hard to fix. If I have a reason for what I'm working towards I'm very happy giving it everything I've got.

I wonder how many other people 'tick' this way. I wouldn't be surprised if both feelings of useful accomplishment and needing information are pretty common drivers for people. Maybe this is something to keep in mind when leading teams - ensure there is a sense of progress, provide recognition and give the team all the information you can about the bigger picture.

Buying people chocolate doesn't hurt either :)

Shouting at problems

I was sitting next to one of our technical services guys (lets call him Dave) releasing a website this morning when a senior manager (lets call him Warrick) came up and proceeded to suggest, in fairly strong terms, that a problem with another client's site needed solving. The suggesting then turned to ordering, blaming, complaining and swearing fairly quickly and with a lot of repetition along the way. Dave handled this well, attempting to explain what had happened while being talked over, nodding and agreeing, not raising his voice at all until Warrick at last went away promising to return "when you've finished helping Jo". I then had the fun of finishing a release with a fairly angry techie who informed his team members that "it might be a good idea to look at such-and-such, at some point today".

Before I go any further: this is a very unusual event where I work. As a general rule I am witness to some fantastic examples of leadership and professionalism from some truly awesome people who care deeply about not only what they do but also the people they do it with. Got that? Good, moving on.

The results of this morning's exhibition were, as far as I could see, two angry staff members (Dave and Warrick), one surprised and disappointed staff member (me) and a decreased motivation and likelihood of the job being done quickly. The latter is most notable since I'm sure this is precisely the opposite of what Warrick intended. In Warrick's defence he was obviously under a lot of pressure from a client who were understandably upset that something wasn't working, and it's always difficult to keep your cool when you're being yelled at and are dependent on someone else to solve the problem. But what a difference it could have made if he had. Perhaps, if the issue and its impact had been calmly and briefly explained Dave would have gone back to him for more detail and to resolve it immediately after finishing his current task... Perhaps he would have explained the problem to others whose co-operation he needed in the same calm clear way and they would also have responded... Perhaps no-one would have needed to raise their voice at all...

Key points I see here:

• explain problems calmly no matter how much they are stressing you out - people will be more willing to help you if you keep them feeling positive about the work
  
• explain the impact of the problem, not "the client is yelling at me" but "this is a big problem for this client because they need the system to order their morning coffee" - people, again, will be more willing to help if they understand why it's important
• be aware of what else the person is doing at the time - you will later reap the benefits of the brownie points you earn for keeping your description brief and thus minimising your interruption of their current task. Not to mention that the less time you spend interrupting their current task is less time it will take to complete and they'll be able to work on resolving your problem all the sooner

Perhaps all of that is pretty obvious, but the next step in the morning's drama was also interesting: one of the first comments from Dave after Warrick left was "This has been managed so badly, Warrick just left things with people without telling anyone else about it." Two things stand out to me in that one sentence: problems can be avoided with careful management and communication is critical. Careful management is all about planning ahead and making sure the appropriate people have the appropriate information. How you give those people information is important too - if you spout some information at one person then walk away assuming they now know everything they need a number of things may happen:

1. they forget all about it
2. they write down most of it but miss one crucial point which results in everything going pear-shaped later
3. they have perfect auditory recall but something happens to them and someone else who knows nothing has to pick up the job
4. they have perfect auditory recall and manage to complete the job

Hopefully you noticed that only one of those options actually involved the problem being solved. Putting things in writing is great. The process of writing something down clarifies your thinking and may help you see something important you had missed. Written material can be picked up and read by someone else, ensuring that when Bob gets run over by a bus all your information is not dragged under the tyres with him. Writing also provides a basis for clarification, where both parties know what is being discussed, and it can be easily added to or corrected when something else comes up.

Incidentally, Dave said much of this to Warrick this morning but Warrick responded by emphasising again how important it was that Dave ring his colleague (whose first child recently arrived) to get the information about the issue. Sigh...