8 February 2017, by Harry Cummings
This is part of a series of blog posts on code reviews, based on two sessions of an internal discussion forum at Softwire. See the first post in this series for more information. In this post, we’ll discuss improving the interaction between the reviewer and the recipient of the review (i.e. the developer).
How do we mediate reviews?
There are all sorts of ways to mediate a review:
- Specialist review tools like Crucible and UpSource
- Repository systems with review features, like GitHub, GitLab, and Gerrit
- Sending diffs/patches over email
- Talking through the code face-to-face, e.g.
- Have the reviewer sit with the developer at their computer
- Reviewing code as a team around a big screen
- The old-fashioned way: Print out the code, stick it to the wall, and scribble over it together
- Pair-programming is arguably a form of code review, taking place as the code is written
Broadly speaking, all of the above approaches fall into two categories: asynchronous online reviews, or in-person reviews. Which kind of approach people preferred was probably the most contentious part of our discussions at Softwire. There was a range of opinions here, highlighting benefits and drawbacks to each approach.
Asynchronous online reviews vs. in-person reviews
Several people made the point that in-person reviews can be more useful for training, and for knowledge sharing in both directions. Face-to-face discussions make it easier to provide context around the code changes. They also give the developer a chance to talk the reviewer through the changes in a sensible order, or perhaps commit-by-commit. This might be better than the arbitrary order in which changes are presented by a diff/patch or an online review tool.
In-person reviews may also provide opportunities to pick up other context that might not be directly relevant to the code quality but is useful for the reviewer to know. For example, any frustrating obstacles the developer encountered while working on the task, which the team might need to address. Reviewing in-person can also save developers from context-switching. If you have enough reviewers on a team, developers can get a review as soon as they finish their code rather than starting on another task and subsequently switching back to deal with review feedback. This obviously comes at the cost of the reviewers having to make themselves highly interruptable though.
A lot of the literature on code reviews also favours some kind of in-person reviews. Here’s one particularly strongly stated example:
“Effective code reviews are not done via a tool or remotely—they are done when you’re sitting side-by-side with the person or pair who just wrote the code. This personal way allows you to share and teach much more information than you can pass in a text-based tool. Don’t skimp on this! If you’re going to do code reviews because your code sucks, do them right.” – Roy Osherove in Notes to a Software Team Leader
On the other hand, some of our reviewers felt that asynchronous online reviews were better partly because they don’t provide the reviewer any additional context. Online reviews arguably make for a more authentic review of maintainability (future developers on the project probably won’t have the luxury of talking through the code with the original developer). Also, coming at the review from their own angle might allow the reviewer to spot issues that the developer has missed.
One major advantage of online tools is that they leave a permanent record of review comments. Some tooling combinations make it particularly easy to go back through old reviews (for example, Crucible with JIRA integration). Several people had worked on projects where they had benefited from the ability to do this.
Several people found it useful to mix online and in-person approaches, perhaps depending on the nature of the change. For example:
- Performing a high-level review in isolation first, then talking through with the developer for context, before finally performing a line-by-line review online.
- Saving face-to-face reviews for bigger or more complex changes
- Carrying out most of the review discussion in person, but using an online tool to track this. That is, initiating the review process and documenting any important outcomes of the discussion.
Reviewers: Making code reviews better for the developer
Quite a few people found phrasing review comments to be a challenge, especially when using online review tools. Some of our reviewers were concerned whether we did enough to make new-starters comfortable with the process, and to make it clear that they can and should challenge their reviewers. After all, the developer is always closest to the code and knows it best. It can be worth a reviewer (particularly one in a more senior position) reminding the developer of this explicitly.
Ways to make reviews more positive included:
- Phrasing review comments as questions or suggestions rather than statements
- Talking through major issues in person rather than writing lengthy review comments
- Talking through and fixing batches of very minor issues in person, rather than writing lots of tiny review comments
- Remembering to always make some positive comments (especially in reviews with some criticisms elsewhere)
This might be more or less important depending on the developer. Generally, reviewers should be conscious of the recipient of the code review and tailor things to them. Extra tact may be required when reviewing code from external third parties, which can be politically awkward.
Developers: Making code reviews better for the reviewer
This wasn’t a question we had set out to answer with our discussion. However, people naturally mentioned how they approached submitting their own work for review, and several common points arose:
- Performing their own review of the changes first (ideally in the same review tool the reviewer will be using)
- Linking to any relevant context (e.g. the relevant ticket in the project’s issue tracker)
- Keeping auto-generated files out of the review (if appropriate and the review tool allows)
- Splitting into sensible commits
- Especially keeping big renames, file moves, or other refactorings separate
- Also splitting code changes across multiple commits where appropriate)
- On one project, we experimented with commiting and reviewing one test at a time, resulting in many small reviews. On a small team, this turned out to be a very effective workflow.
As we saw in the previous post, there are many different valid approaches to code reviews. Organisations should give teams the flexibility to choose a code review process that meets their needs. The first post in this series covered the wide and varied benefits of code reviews. As a team, you should reflect on your code review process, considering what value it provides and what further value it could provide. This will allow you to evolve your code review process to be more effective. I hope this series gives you some ideas that you find useful. Please feel free to share your own ideas on code reviews in the comments below.
1 February 2017, by Harry Cummings
This is part of a series of blog posts on code reviews, based on two sessions of an internal discussion forum at Softwire. See the first post in this series for more information. In this post, we’ll cover some of our current approaches to code reviews.
We tend to at least implicitly perform code reviews in multiple passes. These break down into three stages:
- “Outside-in” preliminary review
- Reading through the original user story or defect
- Reviewing the design
- Checking out the dev branch and doing some cursory testing (this can be useful for reviewing UI issues or things that are hard to spot from the code or by automated tests)
- Reviewing the tests at a high level (do they function as good developer documentation for the code)
- Review of the code itself and the tests in detail
- Review of any activities surrounding the code change, e.g.:
- Manual testing
- External documentation
- Risk/impact assessment
Note that not every project needs all of these passes. The point is that “code review” is a broad term covering a range of activities. Which activities you carry out, and when, may vary by project. Although within each project, there’s a lot of value in being consistent. Consistency helps developers become comfortable with the review process, and makes code reviews a much more reliable tool for quality assurance.
When do we review
As noted above, different code review activities may be carried out at different times. There was a general consensus in our discussions that reviewing earlier is preferable. Most projects insisted on at least some form of review before commit, although a few relaxed this in special cases (depending on the type of project) to avoid becoming a bottleneck.
About half our teams are actively performing up-front High-Level Design reviews. These can be useful for everyone but especially for less experienced developers (which might just mean less experience with the particular project). They encourage working through design issues up front, avoiding wasted time at the implementation stage. It also means the code reviews can then focus on just the code. The only problem mentioned with HLD reviews was that it can be a bit unclear what we mean by an HLD, and sometimes people go too low-level. For projects broken up into well-sized tasks, an HLD could just be a couple of sentences and a few bullet points.
An alternative to up-front HLD reviews is reviewing roughly implemented code, essentially a spike or proof-of-concept. This can be particularly useful on tricky legacy codebases, where it might be hard to see how to go about introducing new functionality.
Who carries out reviews?
Most of the people in our discussions were their team’s technical lead. Unsurprisingly, tech leads were doing reviews themselves, but there was a lot of support for reviews being done by not only the tech lead. Getting more people involved in the review process is a good way to build people’s confidence, share knowledge within the team, and help people become more comfortable with the review process. One person doing all the reviews can also become a bottleneck and slow the team down. Perhaps more importantly, giving developers the autonomy to carry out genuine peer reviews is a show of faith in the team’s ability, and makes it easier for reviews to act as a positive motivator.
One problem with having multiple people involved in reviewing is that it can become confusing for developers. It’s not always clear how to pick an initial reviewer, or when a review would be considered “done”. It’s important for each team to agree on a consistent approach, although approaches can of course vary between teams. Most of our teams use one of the following approaches:
- Let the developer choose the initial reviewer and allow the developer or the reviewer escalate to the tech lead if needed
- Have a high-level second line review as part of the standard review process
- Include the tech lead on every review, but allow developers to merge their changes as soon as at least one person has reviewed it. This prevents the tech lead becoming a bottleneck but still gives them a chance to go into detail on any red flags.
All the above approaches include the possibility of the tech lead acting as a second line reviewer. Our tech leads would go into more or less detail in their review based on the nature of the change and the experience of the other people involved (i.e. the original developer and reviewer). In some cases perhaps just reviewing the comments from the first-line reviewer and/or looking out for changes within common problem areas of the codebase.
How much detail to go into in a second line review is a matter of judgement and may not be obvious. It can help to think of the goal of reviewing as gaining trust that the code is up to standard, and getting involved enough to meet this goal. Of course, it’s still worth bearing in mind the importance of code reviews for training and mentoring. A second-line reviewer may be looking out for learning opportunities for both the developer and the initial reviewer. They’re also in a position to assess the quality of the interaction between these two roles. This will be the subject of the next post.
25 January 2017, by Harry Cummings
There is a lot of writing on the importance of code reviews and how to go about them at a low-level (i.e. what to look for in the code). This series of posts takes a higher-level look at how to approach code reviews and how to maximise their benefits. We’ll also refer throughout to other useful writing on code reviewing and how to make the most of it.
At Softwire, we have always carried out code reviews in one form or another. But our methods and tooling have evolved over time, and often varied between projects. This allows each project team to find a way of working that best suits them.
We recently ran couple of internal lunchtime discussion forums to talk about code reviewing and pool our collective experience. The goal here wasn’t to try and agree on “one true code review methodology”, but just to share ideas between teams. We discussed the perceived benefits and overall aims of code review, and how we go about achieving these.
It turned out that the approaches to code reviews within the company are even more varied than I’d thought. But there was some broad consensus on the benefits and aims of code reviews. However, the benefits that we valued most may surprise you.
What value do we get from code reviews?
Both of our discussions came up with a similar consensus on the range of benefits provided by code reviews. In rough order of their prominence in the discussions, these were:
- Training and mentoring
- In fact, several people felt that the healthiest approach to code reviewing was to treat it as a training opportunity first
- Knowledge sharing
- In both directions (i.e. both the developer and the reviewer had opportunities to learn from one another)
- This includes sharing domain knowledge, general technical knowledge, and knowledge of the specific codebase
- Encouraging people to produce better work (knowing that it will be scrutinised by your peers)
- Keeping the codebase consistent, in terms of style and structure
- More generally, keeping the codebase maintainable
- Catching defects, or at least catching them earlier (and so making them cheaper to fix)
One point I found interesting here was that “catching defects” was one of the last points to come up, in both discussions. There was a lot more emphasis on the holistic benefits of code review: training, knowledge sharing, and improving the overall codebase.
Quality assurance is the main focus of some widely-referenced sources on code reviewing. For example, Jeff Atwood’s blog post Code Reviews: Just Do It, and the two books that it references (Peer Reviews in Software and Code Complete). These mainly focus on reducing errors/defects/bugs, and only briefly mention other benefits. Of course, improving defect detection is not a bad argument for doing code reviews. Perhaps it’s also the most persuasive one in organisations that don’t have a history of doing code reviews and are reluctant to let people start spending time on it.
Defect detection is a valuable and somewhat measurable benefit, which may make it a good angle to sell the idea of doing code reviews at all. However, for the developers and tech leads who actually carry out code review, it doesn’t need to be the only aim or even the primary aim of the exercise. So how do we go about code reviews at Softwire? We’ll cover this in the next post.
22 December 2016, by Jiang Yingxin
Check out all the awesome fundraising events we’ve held in the last 2 months!
We happen to have lots of amazingly talented musicians here at Softwire, and this year our top lead guitarist Harry organised our popular Charity RockStock in aid of mental health charity Mind, and WarChild, a charity based right next door to us in Kentish Town who are doing fantastic work protecting the rights of children caught up in war. We had a blast and raised nearly £600, which was doubled for a total of £1200 under Softwire’s generous charity matching scheme!
As usual, our resident quizmaster and commercial director Tom wrote and hosted our annual quiz in aid of Médecins Sans Frontières. As a result of his reputation for setting really fun and interesting quizzes, his persistent marketing campaign in the weeks leading up to the event, and his glamorous assistant Lachlan’s raffle-ticket-selling skills, we raised a whopping £4,850 for MSF after Softwire’s doubling.
The Great Softwire Bake-Off
Our kitchen team Helen, Dom and Massimo have been hosting regular Charity Breakfast Clubs for Refuge. This month they put on something extra special – to help us through Bake-Off withdrawal the week after the final, they hosted a lavish bake-off with plenty of cake, tea and cocktails for bakers and non-bakers alike. We had lots of stunning entries from various Softwirians an impressive chequerboard cake, mille-feuille, and a gingerbread house beautifully decorated by our director Dan’s two children. Star Baker went to James with his delicious fruity sponge cake. With Softwire’s matching, we raised a total of £1,400.
We’ve been holding regular Charity Saturdays since our founding director Dan came up with the brilliant idea three years ago. It’s a day in which we all come into the office to do what we do best – a normal day’s work – and Softwire donates all the money earned by the company that day to charity. It’s incredibly efficient and is by far our most successful fundraising event each year. We raised nearly £11,000 in just one day! The money went to our usual favourites – SCI, Ashanti Development, Home-Start UK and Giving What We Can.
Christmas Jumper Day
To round off the year, we showed off our silliest Christmas jumpers for Save the Children during our annual Christmas pub lunch, and raised another £145.
We’ve raised over £18k in the last two months, for a year-end grand total of over £30k, and we’ve had lots of fun doing it too!
21 October 2016, by Anna Tindall
As a second year Computer Science student at Cambridge without much free time, I had learnt a lot of theory but had done surprisingly little useful programming. I therefore applied for a summer internship at Softwire to change this. I wanted to gain experience working a proper project from beginning to end and Softwire did not disappoint. I did a four-week training internship starting at the end of July.
6 June 2016, by Emily Penycate
At Softwire the staff are introduced to a lot of interesting new activities, and Thursday 14th April was no exception as Softwire London’s Chillout was transformed into a swinging dance floor from the 1920’s!
Staff donned their glad rags, rock-stepped and shuffled to their first swing dance lesson, provided by the Mudflappers’ teacher Olly. The class was aimed at complete beginners and the hope was to provide people enough simple moves they can string together to make a number to any swing song. Some of the class were lucky enough to have done a fair amount of partner dancing before; like Suzanne who does West Coast Swing every week, and Dom who has been to the Bristol swing dance festival a couple of times!
After a funky warm up which had everyone groove stepping around the room and waggling their hands over their heads, the group partnered up and learnt about 6 and 8 beat steps. Swing, and in this case ‘lindy-hop’, is in 32 beat phrases; this means you need to select the right number of 6 and 8 beat moves to finish with the end of a phrase. Olly armed everyone with a 6 beat basic (twice), a tuck turn, a bring back and then a fancy finish called ‘Johnny’s Drop’ to which the group responded with great enthusiasm and abandon.
Swing dancing, like salsa and tango, is a ‘led dance’ which means the class split into leads and follows. Traditionally this is split along gender lines, in true Softwire fashion everyone got a go at both roles. This created the occasional interesting pairings like Dom 5’5” leading Alex 6’7” in some pretty spectacular twirls!
Everyone involved had a great time and have decided to head along to Olly’s normal lesson, in Dalston, in the coming weeks.
23 March 2016, by Vikki Vile
At Softwire they’re very lucky to have freshly prepared lunches cooked for us every day by our amazing kitchen team, Helen, Dom and Massimo. Undoubtedly this is something that hugely contributes to what makes Softwire such an awesome place to work and helped us achieve 14th in this year’s Best Small Companies to work for.
Our kitchen team is committed to sourcing the best and most sustainable fresh ingredients for our lunches. When they are not cooking for us they are always looking for new ideas, suppliers and products to enhance and improve the lunches on offer.
Apart from baking bread and making pasta (actually they do sometimes do this as well!) the kitchen team pretty much prepare and cook everything we eat in the Softwire kitchens. They use fresh herbs, seasonal ingredients and quality suppliers to deliver lunches that are a highlight of our day.
In order to consistently offer us such high quality, all food is sourced from local North London suppliers; eggs are always free range and from reputable producers (Clarence Court or Black Farmer), fresh chicken, pork, lamb and beef is always free range (supplied by Meat Naturally) and fresh vegetables delivered daily. Seasonal vegetables are used whenever possible to minimize food miles and to maximise flavour and value for money. Cheeses and dried goods are sourced from Carnevale (Italian supplier) and Ocado. Our kitchen team always select the products that offer the best quality and flavour and will use organic whenever possible to produce roughly eighty delicious lunches every working day.
16 February 2016, by Jenny Mulholland
Following on from our managing director Zoe Cunningham’s blog post on getting more women into technology, I thought I’d share some of my own recent experiences around encouraging women in, and into, technology.
21 December 2015, by Laura Bethke
For the third time in its history, Softwire hosted a Charity Quiz in late November this year, with all proceeds donated to the Schistosomiasis Control Initiative.
As in previous years, the quiz was written and hosted by director Tom Steer, whose general knowledge is so great that he has probably been banned from all pubs in the area. He can now only use his quizpertise to selflessly write quizzes for other people, and he does that very well.
There were four ordinary rounds, one table round (where the teams had to identify films from cryptic descriptions, books from their first line and words from their chemical element names) and as is traditional for Softwire quizzes, a taste test. Tom lovingly prepared pick and mix bags full of jellybeans with 9 different flavours for each team, and this was probably the only time in history anyone ever ordered a 2kg bag of liquorice flavour – I can assure you they are disgusting.
In London, 13 teams competed for the crown, while our sister office in Bristol held a parallel event with 4 competing teams. It was a tight race with the three leading London teams, Flux Quizpacitor, The tall and the wise and Quiz Akabusi being within 5 points of each other after two rounds and the table round. However, an absolutely stellar performance with 25 out of 25 possible points in round 3 eventually secured Quiz Akabusi the victory. In Bristol, Quiztosomiosis Control InQuiztive was the winner.
The best team name of the night should definitely be awarded to “Hopefully not last”; even though they did not try to come up with a clever quiz pun, they did manage to hit their goal exactly and end up in 12th, out of 13 possible places.
The quiz was also accompanied by a raffle, with half the money donated to SCI and the other half up for grabs by a lucky raffle winner, should they manage to answer a final question. Rich Bradley, part of the winning team Quiz Akabusi, had a very lucky day indeed with his ticket being the chosen one and he managed to answer correctly that the Dachshund was originally bred to hunt badgers (an easy question for any German speakers, as it literally translates to “badger dog”).
Overall it was a fantastic night in the office, with lovely food prepared by our fantastic kitchen staff, and with a really entertaining quiz that managed to attract enough interest to raise £1347.50 for SCI! Hopefully we can convince Tom to carry on writing quizzes so we can host similar events for many years to come.
28 October 2015, by Chris Arnott
There have been a lot of stories about how there are many benefits to standing all day rather than sitting in an office. But how about a walking desk?
We’ve offered employees the opportunity to use standing desks for a while now, which we provide using some carefully selected IKEA parts (a coffee table and a shelf) and with a small amount of effort result in a great desk for this purpose.
However, recently we decided to take this a step further and set up a walking desk. There are lots of commercial options out there, but as they are quite expensive, we decided it would suit our needs better to build our own. Fortunately we have a very good handyman who helps out with minor tasks around the office, and so we tasked him with building us a desk that:
- Fit over the existing treadmill in our gym
- Had a large work area
- Was adjustable, so that it would be practical for people of all heights
We’ve been using this desk for a while now, and personally, I find it great for maintaining focus, and particularly useful for finally getting round to that task you haven’t been looking forward to all week.
Walking and working helps brings clarity to my thoughts and although it makes my handwriting near illegible, my typing is barely affected. The walking quickly feels natural and it’s perfectly easy to do an hour or so walking along at a slow pace.
The disadvantage of the walking desk is that it can be quite noisy. We got around this by installing it in our gym to begin with and we’ve now moved it to a more permanent home in one of our meeting rooms.
Overall, we think that walking desks are a great idea, and although no employees have planned to switch to one on a permanent basis, it works very well as a hot desk. So if you have the time and skill (or money) to try one out. I’d definitely recommend it!