Lessons Learned and Experiences doing Code Review

Today I want to share some experiences and findings in regards to Code Review. Code Review is a practice that everybody does it today. I was doing code review way before Github existed. I will share experiences before Github and after Github. Difference companies and different teams have different realities and preferences. I don't think there is universal or wrong but this is my experiences. I never was and still not a big fan of tools for code review. This subject is very related to Technical Debt. I think there are 2 kinds of technical debt the one you pay often and the ones you need to schedule because they are big and might need lots of time to fix it. However, if you just managed tech debts often means you never pay it. Also, you should be paying even if no one outside of the team is asking for it. When you are doing a PR is a good time to pay some debts related to that PR, also long as is not too much. I found Github view for code review very limiting. Small config changes are great but for bigger functionality is hard to know because you just see the deltas, off course, you can navigate through the whole code but that's not what the main UX push you todo. So I end up doing a code review in 2 different cycles. When someone in my team submits a PR I will check it but also time to time I would look the whole code repository.  After looking at the whole code base I create a classic ".TXT" file with a list of all things that could be improved. IMHO on this second cycle, I see stuff that I do not necessarily see on the first cycle(when PR was fired - talking about Github).  You might say that a tool would make it easy to manage it, but you know, if your architecture if right you don't have 100 people touching the same code base so there is no need for a tool. However, if your architecture if a big mess, yes you need more tools. Some platforms like Mobile tend to be more monoliths and some files are hard to split and more modular. I did code reviews in Java both Backend and Android code, IoS(Objective-C), Python, C, C++, Java, JavaScript, Bash, Scala, Go and Python Code. Most of the time I did Java and Scala code reviews - So you will see things towards Java/Scala here.

Expressing Debts through the code

IMHO code review is something that needs to be close to the code, that's a very XP(Extreme Programming) way of think. Talking about Java for instance in the past I created a set of annotations so I and my team could express technical debts on the code. This was a big hit back on the time. Back on the time, you could express on the top of class or method something like:

@Refactoring
So everytime someone open that class it would see a note that they need improve that code. Why create an annotation if you could create a comment with //TODO: NEVER FIX ME. People don't fix TODO notes. :D  So yes that was rebranding todo as java annotation. I think It worked out back on that time because of 3 reasons:

1. My Team cared about CODE quality and that feeling was created and re-forced by the team.
2. I made a simple ruby script that downloaded the source code and counts how many Annotations I had so I was running that every week and asking the team to reduce the number of Tech Debts.
3. Every simple Code Review that happened as required to reduce the number o Refactoring annotations.

Back on that time, I had no Github so Code Review was done time in that TXT files, since I was doing more continuous integration approach, and yes, no branches :D Them github come with a bunch of people doing git flow(which I still think is complicated and don't add value) and Git Hub Flow(which is simple and adds value).  Today I don't use this anymore because I work with: Java, Scala, C, Python, Bash a lot and I would need to create something like that for each language, plus the fact that my team is more senior than ever so nobody needs to say things 2.x :-) Having an sr teams means that everyone can merge PRs. IMHO JRs teams should not merge PRs as shared responsibility but concentrate in Sr member this way who knows more, in theory, does better review. Github is great for this, even for private repos, even if you don't have to merge power you can comment which is great. 

Review PRs in private repositories

The first thing that happens here is the fact that now you have a PR where you can put comments and its easy to express what you want to change. I worked with repositories that lieraraly was ultra shared and the whole company touched and I experiences worst times ever accepting prs and repos where there was more discussing and comments them commits which might sound as something code but actually was not. 

Some people believe in PURITY like you cannot put a single line of "bad" code or "style" in your code base. I'm not like that. I believe in Waves and Progress Principle. You don't need to pay all tech debts at once, you don't need to fix all the things in the same PR. It's good to make progress. Having a PR sitting on the repo for days and they receive a list of 50 changes is the wrong thing that could happen. So there are lots of waste doing things that way and I do very bad for the team morale. 

Kanban W.I.P Limits applied to PRs

Took me some time to realize but( I don't know how I forget this :-)), Reducing WIP increases the Throughput, a very Lean Kanban principle. How we do we limit WIP in PRs? Limiting the number of PRs? Well, that's one way but I discover that limiting the number of FILES was way more effective. Currently in my team and the repos we "own" LIMIT PRs to 10 files MAXIMUM. There are exceptions like you need rename the project this will make you go beyond 10 files, however, it is a feature or bug fix we limit in 10 files. This also creates several benefits like: 

1. We end up spending less time in code review
2. Code Review is way more easy and easier to understand
3. People end up finishing more tasks(Progress Principle)  - this creates a positive feeling
4. More PRs means more tasks so you need split your work - also removing waste
5. Merge time is very fast and nothing sits on PR queue(Kanban) and (Github)
6. Feedback travel fast and speed up delivery and fix time

One thing my team does a lot has discussions on Slack rather them in Github because is faster and don't pollute much the tread. Sometimes we need to have 10-15 minutes chat to decide whats best todo. I guess this is a matter of preference but Slack is faster to time them via github comments. Sometimes we are in different timezones with a 4-6h difference so we end up using Github more. 

Clean Code / Space VS Tabs and Other ways to waste time

There are some properties that are no doubt important. I try to separate this props from "style" as much as I can. I don't see value creating a huge style convention on wike or IDE file that is never updated and people stop maintaining over time. IMHO What matters:

• Performance
• Proper Error Handling / Fallbacks
• Code Reliability
• Avoid unnecessary Classes / Methods
• Typos that you might miss it
• Basic Clean Code

Why do I say "Basic Clean Code" because I mean not have a method of 100 lines of code or having 20 if statements in the code Basic Clean Code also stands for some "reasonable" naming and avoiding very long names that mean nothing This might get subjective and people think different there were might a trap happens and you end up having pointless philofhial discussions. So there is some level of "Bad Clean" code which I try to avoid. Spaces vs Tabs is a pure waste of time discussion for me and really does not matter. IMHO the only thing that matters is people use same chars for tab or spaces, that's the only thing. IMHO the big thing is to have as many methods as possible because with methods you can explain what your code does, so you explain by telling why you are doing something. 

Code Review need to work with Tests(Unit and Integration) - Currently, on the last 4 years I'm not working just with microservices but with DevOps and NoSQL Engineering so Integration Tests matter a lot for me(On microservices often unit tests matter more). Tests are nothing without Clear Design and Proper Architecture so this is just one element of many elements of an asuccessfull solution. 

Cheers,

Diego Pacheco