Code Reviews 4: Three Principles for doing effective Code Reviews

In order to do effective code reviews I propose the following three Principles of Code Review that every professional developer in your team must sign up to.

Code Review Principles for Developers

1. Nothing gets merged to the master branch without the approval of a code review. Ever. Treat master with respect.


2. After two¹ approvals have been obtained, the author merges work to master.


3. When I take on a fellow engineers' code review I must progress the review immediately as the most urgent item on my list.

Reasoning the principles

Professionals operate by certain principles and they apply them consistency and fairly. Let's explore some of the reasons behind why each of these principles are important.

1. Nothing gets merged to the master branch without the approval of a code review. Ever. Treat master with respect.
1. I am human and I make mistakes. I require the validation by my peers on what I have done.
2. As a professional, I have a duty to educate others. A peer review is one valuable way to share knowledge.
2. After two* approvals have been obtained, the author merges work to master.
1. The author should be responsible for the full completion of their own tasks.
2. The author asks for and appoints people to complete the review. At least one should be senior and experienced. Those appointed people stay with the review until it is complete i.e. merged to master or abandoned
3. All actions between the authors and reviewers are closed pragmatically with necessary rework inspected and complete. Code is never merged with any outstanding code review comments. All review comments are either implemented or discussed with reviewer and agreement reached as to why comment is not acted on. The decision and the why should be recorded in the source code as a comment.
4. Sending the review to the entire group leads to confusion as to who is doing the code review. IF everyone is responsible, then no one is responsible. Be clear who exactly is doing the review and appoint them to the review.
3. When I take on a fellow engineers' code review I must progress the review immediately as the most urgent item on my list.
1. This is context switching, but we value “done” tasks over “work in progress”. To help flow of tasks in my team, we realise that people at code review are closer to “done” than I am – so unblocking them is my top priority and the context switch is more valuable.
2. If I have more urgent items to tend to, I do not take on the code review.
3. Some one must take the code review, so the team might need to discuss priorities.
4. If I do this review now, when my turn comes I can count on my team mates to drop their tasks in progress so that my code review progresses rapidly.

---

1. I recommend two. You may choose whatever number suits your project. But when you choose – stick to it consistently. This is so we are fair to every developer working on the product. Picking too small a number, means we don't get enough feedback. Picking too large a number requires interrupting more tasks in the team to progress the task in review.

---

Thanks for contributions from: Rachel O'Toole, Shane McCarron, David Hatton

---

Pair Programming: Why isn't it more widespread?

In my experience we've been pretty good at adopting XP. We agree, by and large, that co-location is a good thing. We seem to have difficulty agreeing a coding standard, but mostly we do. We struggle with writing unit tests first - often we are writing tests to hit a “coverage target” and after the fact, but again most people agree we need to write automated tests. We agree that we need to integrate more often, requiring automated integration tests and the adoption of a continuous integration system for our products and this doesn’t generate too much debate. 

But after twenty years of extreme programming practices, the one practice that seems to have the least widespread adoption is Pair Programming.

So why don't we pair?

In my opinion our resistance is rooted in three different causes; Organisational, Personal and Societal

Organisational Resistance

Halving our number of people
Simple. We cannot afford to have two people working on one thing. We couldn’t be efficient doing pairing. Many managers just can’t see the sense in it.

Rewards, Recognition and Promotion
All our corporate models for reward and recognition are based on the individual, not pairs. From performance reviews to bonuses, remuneration and promotion, all these are based on the individual. You are given your task(s). How well you executed these tasks and the more of them you get done on your own, the better you are rated. This feeds into your exam/evaluation results. Ultimately your personal results and successes are reflected in higher monetary return. Most jobs and most industries are set up along this "Motivation 2.0 Model" – the better you do, on your own, the more reward you will have coming to you. If you need help, that kind of counts against you.

Personal Resistance

Loss of Freedom
A big challenge to overcome in pairing is the perception of giving up a large part of your personal freedom in work. The day we joined a company we were given a desk - your desk, your own piece of real estate. You are told when you are expected to be in the office but how you manage your time in the office is your business. You dictate when you go to the bathroom, when you take a 5 minute break to browse the daily paper or use social media… There is great satisfaction in having that autonomy and freedom within the confines of a professional environment. 

When we pair there is sense of obligation to be at either your desk or your pair partner’s desk during the working day. You will have to excuse yourself when you need a break. You perceive that you are accountable, on a social level at least, to someone else.

Arrogance
You’ll be the one giving everything and you'll gain nothing in return. You feel that your expertise is getting diluted and it's the main reason you're valued by the company.

Imposter syndrome
Can you live up to your own reputation in the eyes of others. You are an Engineer with many years experience and many projects behind you, but they are all on older technologies.

Social difficulties
Pairing is socially difficult. You are forced to work closely with someone you might personally dislike either for physical or psychological reasons. We are social animals and we all carry our own biases, conscious and unconscious. We all have our own personalities and we tend to like people with similar personalities and dislike those that are different.

Societal resistance

The vast majority of roles and jobs in our society do not use pairing. In fact there are a only a very few roles where pairing is widespread and established. A quick google test of “jobs that are done in pairs” returns jobs for “au pairs”, something entirely different. This means in general people are not well equipped or well practiced with the skills that pairing demands on the participants.

Introducing Pair programming to your organisation

To successfully introduce pairing to your development organisation, you have to design your change program to address and overcome all these aspects of resistance. And for some people their resistance is so established in their minds, that it will take a significant effort to bring them on board.

---

Thanks to Contributors: Shane McCarron, Rachel O'Toole

Assumptions I held about software development, that no longer hold true for me

That architecture is more valuable than test

1. Often what we do with software doesn't change that much. Good tests describe what we want to solve with software.
2. Software changes greatly in many directions over time. The architecture we choose today will not "stretch" in every direction of change, requiring significant changes at some point.
3. We can only sell features. Tests are a great description of features. We cannot sell an architecture.
4. The right tests, written at the right level in the application will out live the architecture.

We need to re-write insert any software product from scratch

1. It nearly always takes significant time to re-solve all the simple problems the old system does for your users.
2. The expectation of the user is immense.
3. Software is not construction, the foundations of our system are not in concrete. It's our own perceptions of the complexity of the current solution coupled with our desire to embrace new tools, that limits the changing the current solution. We think it's easier to start from scratch.
4. One of the Facade, Adaptor or Proxy design patterns should be employed to bring fundamental change to a software system.

That you can be faster if you do Agile in software development, but keep all your traditional stakeholder, requirement, release and business management processes.

1. Agility means you are agile from the customers’ point of view, i.e. they are getting something delivered from you every 1-4 weeks.
2. It's hard to release every X weeks, if you can't put work through your production pipeline in less than X weeks.
3. The overhead of breaking work down to fit into short sprints, actually makes the overall work take longer to do in the medium to long run.
4. Agile allows you to maximise the amount of work not done. Can you choose what features not to do?

That I must enter "the zone" to be productive or that I work better on my own.

1. We do require focus in a very busy world. But the zone has no feedback, so we often end up doing the wrong thing.
2. It’s important to come out of the zone frequently so we don’t lose sight of our destination.
3. We do need some quiet time to absorb information and reflect, but this isn’t productive time.

If I am efficient and productive, I am effective.

1. Productive means you are active at building something.
2. Efficient means that you are really great at building something.
3. Effectiveness is measured by the receiver of the output of our work.
4. We often build the wrong thing or we build a lot more than the user needs.
5. It is better to build something small that is used and useful, than to build something large that is never used.

In software development, the "busy" team is the "best" team.

1. It's nearly impossible to visualise the output of software teams.
2. It's even harder to get objective measurement on that output.

Bad developers cause the most bugs.

1. Bad developers usually deliver very little and the issues they create rarely get passed our first test loops, eg Code review and basic integration test.
2. Developers who produce the most code, produce the most bugs.
3. The code that is not written, contains no bugs.

Unit tests test methods

1. Most unit testing is written towards all the methods in every class in our component. It is very tightly coupled to our design.
2. If I have to change the test when I change the source, I can't be sure the method/class/component worked like it did before.
3. The "acid test" for a good unit test is that you can change the implementation without changing the test.
4. Refactoring means that I re-structure & re-write code, but it does exactly the same job it before.
5. Good unit test is written towards the public API.

There are lots of good unit test and TDD articles/books and resources.

1. Most say that for every method you write, you must have at least one unit test. This is wrong.
2. Most say you must design for test. This is wrong. Good tests simulate real use.
3. Most state that the "unit" is the method under test. The unit is the test itself.

---

Thanks to Contributors: Shane McCarron, Rachel O'Toole

Java interview questions - level advanced

This is the third in a series of articles on Java interview questions. These questions require subtle, but important, details of certain parts of Java to be correctly answered. These questions are less obvious than the intermediate questions.

1. Why do we favour the use of StringBuilder over String concatenation?
   String concatenation used to be expensive because it creates more String objects. StringBuilder does not create as many objects and is unsynchronized. In later versions of Java, the compiler optimizes concatenation operator to use StringBuilder automatically on compilation, so it doesn’t matter if you use the “+” operator or StringBuilder.
2. Why is it important to implement hashCode() if we override equals()?
   It is important to override hashCode because the java collections framework relies on hashcodes. Ultimately if two objects are equal they must return the same hashCode, however it is not required that two objects that are unequal produce distinct results, but collections might be more efficient if they do.
3. What is the contract for equals() method?
   It is reflexive: for any non-null reference value x, x.equals(x) should return true. It is symmetric: for any non-null reference values x and y, x.equals(y) should return true if and only if y.equals(x) returns true. It is transitive: for any non-null reference values x, y, and z, if x.equals(y) returns true and y.equals(z) returns true, then x.equals(z) should return true. It is consistent: for any non-null reference values x and y, multiple invocations of x.equals(y) consistently return true or consistently return false, provided no information used in equals comparisons on the objects is modified. For any non-null reference value x, x.equals(null) should return false.
4. If I have two object of the same type. If I pass the first object to the second object, can the second object directly access the first objects private member fields?
   Yes you can – you do not need to use getter methods or any other “fancy” solution. Private is class private, not object private.
5. Outline how you would use a Future interface in Java?
   A future is used to get the result of an synchronous result from a ExecutorService. The ExecutorService will gather the return values of call() and call back the main application via the future interface
6. What is strictfp? Where would you use it/side effects?
   strictfp makes the JVM put restrictions on the floating point datatypes to ensure they are portable. Some processors are able to make more accurate floating point calculations and the JVM spec allows these to be taken advantage of off. If you use it your calculations will be less accurate, but will be equally wrong on all platforms.
7. How many times does finalise get called?
   Finalise gets called exactly once. If I manage to re-reference the object in my finalise method, making it no long eligible for garbage collection - the next time it comes around to garbage collecting the object finalise will not be called.
8. Explain the substring memory leak problem in Java 1.6:
   http://www.dzone.com/links/r/the_introduction_of_memory_leak_in_java.html
9. Whats the difference between Comparable and Comparator?
   Comparable has one method and makes a class comparable to another. The comparator takes two objects of the same type and applies logic to compare one to another.

Code Reviews 3 - As an Author, When am I ready for review?

This checklist for the author calling the review. The checklist helps avoid code reviews that just talk about the trivial stuff.

As an author, I should respect the time that my reviewers are putting into my submission. In that case, I wish to have my work as complete as possible and have a clear list of inputs ready for the reviewer, so as to get the most effective code review possible and not waste any ones time.

Authors's Code Review Checklist

1. Tests complete and passing
2. Code style adhered to
3. Code complete
4. Programmer documentation is complete
5. Code, Test and Developer Documentation is committed
6. All static analysis run and passing

Exploration

1. Tests complete and passing. All legacy unit and integration tests are running and passing. Prove that our changes have no undesired side-effects on existing functionality. Any new tests that are relevant are written, running and passing. We apply the same source control standards to test code.
2. Code style adhered to. I'm not saying that you have to use the same IDE as everyone else, but you should match the style that the majority of the code is written in as this enhances readability. So configure your IDE or run Checkstyle to ensure compliance.
3. Code complete. A no brainer, obvious? Well the code should be complete enough to make sense, run and do something useful. It might not be the complete solution. The author should be pragmatic.
4. Programmer documentation is complete. If you are writing a library, the API, javadoc and sample usages in the SDK are all complete.
5. Code, Test and Developer Documentation is committed. Change management 101. Programmers share via source control. Typically it should be committed on a side branch, to be merged into main or master once the review is complete. The use of source control helps enforce discipline. It's obvious if the author changes the contents of the code up for review and helps prevent confusion for the reviewer.
6. All static analysis run and passing. The reviewer shouldn't be looking for stuff that static analysers should be finding. You should have run PMD, findbugs and/or sonarqube with all pre-agreed rule sets.

Once I have completed all six parts, I am ready to submit my code review to reviewers.

Supporting resources

Clean Code, Robert C. Martin
The art of readable code, Boswell and Foucher
Pragmatic Programmer, Hunt and Thomas
A Guide for Code Reviews
Code Review Matters and Manners
Peer Reviews in software, Karl Wiegers
What to look for in a code review, Trisha Gee

What makes a good check in?

Updated 04/01/2016

Updated 13/04/2017

Updated 23/09/2017

Happy new year 2017

Welcome to 2017! 2016 was a busy year. I spoke a little on programming, code review and a fair bit on teams, agile and communication.

2017 will continue the theme. I'll focus on production of software and more on pairing and code reviews. Looking forward to it!

Evil Two, Three and Four Letter Acronyms

I propose the following three rules for Three Letter Acronyms (TLA)

• Platinum rule: Don't use Three Letter Acronyms. Ever.


• Golden rule 1: Only use Three Letter Acronym's if they are well-known¹.


• Golden rule 2: Don't create new a Three Letter Acronym.

One of the things we must continue to do in software development is to promote understanding. Every 5 years the number of developers doubles, so half the software developers out there have less than 5 years experience.

In our industry², we have a huge love of Three Letter Acronyms. In fact we nearly have our own flavour of English in TLA's. And this is not surprising given the lack of typing skills during the early days of computing (that continues today!). It's a lot easier to repeatedly type TCP instead of Transmission Control Protocol. So we gain efficiency on the writing side of the communication equation.

However TLA's are a barrier for understanding for the reader. Especially for novice and intermediate reader. They are particularly evil when one needs context to understand them or when we create new ones that are local to the document being read. For example: in a telecommunications domain does IP stand for Internet Protocol or Implementation Proposal or Intellectual Property? If I load a module into a CD; is a CD a Control Device or Compact Disk?

Now consider that the reader may not be a fluent English speaker. Consider that the artefacts are written once, but read many times. Shouldn't we favour efficiency of understanding for the reader, instead of the writer?

So to promote better communication I propose the above three rules for software development teams to follow. Let me repeat that first one again. Never use Three Letter Acronyms! Use regular words.

---

1. Definition of "well known": If I pop the Three Letter Acronym into google and it comes up in the first 5 results.
2. We are not the only industry guilty of this.

---

Edited 20th February, 2017

Code Reviews 2 - Dysfunctional Code Reviews

We've all had them; code reviews that just aren't effective. Here are a few scenarios.

I would love to hear your feedback about other types of reviews you have encountered.

As a submitter:

The '0' feedback code review

You eagerly prepare your code, a few methods and a few new tests. Submit it to the guild. Appoint the master code reviewers. The reviewers give it a +2 a minute later, with no feedback what so ever. WFT... surely there has to be something wrong with it?

The "Trivial" details code review

You submit your code to the review board. After a wait, the results come in. The reviewer wants the variables and methods in alphabetic order. They insist on putting in all the full stops on the comments. They are making it a blocker that you don't know the difference between "it's" and "its". They point out you are missing capitalisation in your javadocs. You haven't expanded three letter acronyms on their first use…

The "goes on forever" code review

We submit out code for review. A few days later, the first set of reviewers find numerous "serious" issues. We address all the comments. It goes out for review again. The following week, the next set of reviewers find another set of serious issues. We address all those and our code goes out again. Another week goes by and next set of comments back highlight another, different set of "serious" issues. We address those and the code goes out to the reviewers again. The next set of comments back and highlighting some of the same stuff as the first review, that the third code review made you put in…

The "Self" review

You are using a code review tool because the organisation process requires you to use one. However your team has no real interest in each others work. When you make changes, you put them up for review. But you always push through your own code anyway.

As a reviewer:

The "token" code review

It's 16:40 on Friday. The sprint ends in 15 mins. Bam! Into your mail box, comes a review for a critical feature for this sprint. The change set is long, several classes with a few hundred lines of code. Just remember it has to be in the main branch by 16:45… and that includes any re-work! It only takes a fraction of a second to hit that +1 button and the weekend is yours!

The guy who is always right review

The author is never wrong. How can you criticise this guy? Their code is flawless. Always. Even when you spot a real issue, they won't listen to you. They'll justify the unnecessary, out of scope code in the solution with fancy subjective words like "extensible" and "future-proofing". Some sections of the code might be unreadable to you, but that’s only because you are a lesser programmer and we'll save several nano-seconds during execution because he used an obscure construct. Basically this code is right. You are wrong.

The guy who is always wrong review

This guy just doesn't get it. The solution proposed isn't just a little bit wrong, it's completely wrong. There are no tests whatsoever. There is no concept of the agreed style guide, he just copied a large chunk of code from anther part of the system - changed a few strings and it's the proposed solution?

The death by a 1000 updates' review

So you get the call for a code review into your inbox. You gallantly volunteer to take it on and you fire up the tool. You spend 40 minutes reviewing the problem statement, then the tests and pick your way through the solution adding your comments as you go. Next thing you check the tool and there is an update to the commit. Many of your comments are now null and void due to the changes submitted. No panic. You start picking your way through the new solution again. Thirty minutes later, in pops another set of significant changes, rendering many of your comments null and void... again! So you start the review... again!

The "rutting" code review

A senior developer has submitted their code for review. Another senior developer has taken up the challenge. These two never see eye to eye. The comments come thick and fast with slightly confrontational and inappropriate language. It's a thinly veiled "you are an idiot"! The gauntlet has been thrown down. The submitter is not going to take that sort of "threat" lying down and they respond in kind, in the tool. The insults and accusations go back and forth for all to see in plain text. The tension rises. Other reviewers fall away, afraid of getting caught in the cross fire. Sooner or later either one declares the other one is wrong or Godwins' law will apply! At this point someone else will step in, merge the code and promises to keep these two apart in future!

---

Updated 23rd September, 2016 Updated 3rd July, 2017

Cycle time and Delivery frequency

Delivery Frequency

Delivery Frequency is the count of the number of times we ship the product out of the team to a receiving organisation. That could be a test group or a "friendly" customer or another interested stakeholder.

Generally we favour higher delivery frequencies. The optimum number depends on our delivery infrastructure. This can vary greatly across products. But at a guideline, we are looking to ship more than once per day. On products with long build and test cycles, this can be a challenge. Why this is important:

1. It's easier to integrate smaller changes, more frequently.
2. We get earlier regression feedback on our changes.
3. It can help reduce "traffic jams" at the end of sprint.

Cycle Time

Cycle time is defined as the length of time a story is actually being worked on. The clock starts once the story leaves status "open" and it stops once the story is marked "closed". Usually this is when the story is delivered out of the team. Why lower cycle time is important:

1. We get an earlier demo to the product owner.
2. Better chance of delivering something in the sprint.
3. There is the possibility of more agility from the team. We can change new work planned before the stories are started.
4. Small stories are easier to implement, are easier to integrate into the system and they are easier to regression test.

Relationship of Cycle time to Delivery Frequency

There is a coupling of Cycle Time to Delivery Frequency that is interesting, and observing the trends in both can be indicators on how the team is performing.

Cycle Time	Delivery Frequency	What it means
Low or trending down	High or trending up	Team is heading towards high performing
Low or trending down	Low or trending down	Team not delivering despite closing stories. Need to investigate why?
High or trending up	High or trending up	Team is frequently delivering "undone" work - need to investigate why? Are stories correctly split?
High or trending up	Low or trending down	Team might be struggling. We need to engage the team to discover the difficulties and challenges they are facing.

Code Reviews 1 - Why?

I'd like this to be part of a series of articles on code reviews. Comments and suggestions welcome. There are as many views and opinions about code reviews, as there are developers. These are my views. I hope they make logical sense. They have been formed through my own experience.

Why do we do code reviews?

There are primarily two reasons why we engage in code reviews. Both are equally important.

Code Quality Checkpoint

For any "product", a review should be part of the production process - this goes from a set of accounts to a presentation. Reviewing one's work is a great practice for any craftsman to part-take in. There are two quality aspects that code reviews can help with.

Code Quality

How can I know that the software I have written is "good"?

Code is written once. Code is read many times. Frequent code reviews help me to write great code.

Code reviews help to have a good code base in the product. A "good" code base is one that is readable by a majority of programmers, without too much intervention by the Author. This has been expressed as the number of WTF per minute. We can ensure any style standards are consistently met.

Product/Component Quality

Code reviews are the also important for finding any possible bugs or errors in the solution we have put together. Since code reviews occur early in the production lifecycle, they are one of the cheapest ways of detecting and preventing issues.

Knowledge Transfer

Code reviews are a medium of communication between developers.

When we start out, we share our work with our more experienced peers so that we can learn. As we get wiser, we share our work with our peers to get their validation that we are doing the right thing. When we are at the top of our game, so to speak, with many years of experience we also take an active part in reviews to help pass on our experience and to get exposure to possible newer ways to do things that more recently trained developers will have learned.