The iOS team at YOOX NET-A-PORTER GROUP doubled in size in 2020.
Adding 100% more people to our team means that we can do more and are even more ambitious and hungry than before, but this rapid growth has created challenges of its own, in particular when it comes to developing, merging and delivering features.
So, over the course of 2019 we spent time thinking about how our engineers could best cooperate and release code smoothly, with a focus on fostering a culture of continuous improvement and continuous knowledge sharing across the team.
As a product team, our main job is to deliver features that our colleagues across the business want and need. Sometimes that makes it difficult to schedule in time to polish the existing architecture, especially because it can be difficult to measure the business value of the changes.
That is why it is really important that the devs team is accountable for the continuous improvement of the code base — this enables team to be more productive in the future.
Taking this view, the Opportunistic Refactoring concept becomes a key tenet of our day-to-day. Martin Fowler, who coined the term, describes it as follows:
[..] What this means is that at any time someone sees some code that isn’t as clear as it should be, they should take the opportunity to fix it right there and then - or at least within a few minutes. This opportunistic refactoring is referred to by Uncle Bob as following the boy-scout rule - always leave the code behind in a better state than you found it. If everyone on the team is doing this, they make small regular contributions to codebase health every day.
As a team we agreed on a basic foundation principle: always try to improve the code you are working on, also known as boy scout rule.
Examples of Opportunistic Refactoring are:
- Remove outdated TODOs from the codebase you are working on
- Fix warnings from the codebase you are working on
- Fix Auto Layout constraints from the UI you are working on
- Remove the usage of
selfin the codebase wherever possible
- Add type annotations where necessary, to reduce the time taken to infer types
- Add unit tests to untested code
- Add snapshot tests to untested UIs
To refactor what you are working on is repeated frequently because refactoring is always welcome when it’s related to the feature/issue/piece of code you are working on. And as a general rule, refactoring is always welcome provided we keep the product release’s size under control. This is because it’s quite common that, especially working on legacy code, a local refactoring quickly becomes larger than expected and it can make reviews by your colleagues more difficult.
It’s important to highlight that without a good test coverage the you cannot do any opportunistic refactor with proper confidence of not shipping code with regression. Please treat unit tests code as first class citizens ☺️
Working on a rapidly evolving large codebase, being a good boy scout sometimes is not enough, or sometimes it’s not possible at all because it’d imply too many changes that would slow down the current development and the next release.
So when one of us notices any large piece of code to be refactored or removed we track it on a confluence page to be discussed in our tech meetings. In this way we get two benefits:
- the whole team is aware of what’s going on in the project
- the whole team is responsible to prioritize our next tech tickets to work on
- the product team is involved in prioritisation of tech tickets
Another important aspect to highlight when your team will grow up so much is that it’s really difficult that anyone can review all the PRs (there are times in which we have ~30 PRs open in parallel), so we need to help the team to keep high confidence on what’s going to be merged.
But at the end of the day our main tool is the Pull Requests, so how we raise them and we review the code can change the team approach to the work, that’s why we wanted to create a bit more complex flow on Github instead of only relying on the JIRA flow.
Our Pull Requests structure
I’m a strong believer that to help the team work better together we have to:
- help people to be more emphatic
- help people to understand the value of the self-assessment
This should happen when we are ready to share the code with our colleagues, usually before raising our Pull Requests. That’s the reason why we structured them like this:
- PR Title: the title is the entry point for any review, it must make sense for the colleagues, so our format is like [Task|Story|Bug]/ticket_no: short title; keep in mind that’s also useful when you generate the release note;
- Ticket reference: it’s the anchor to the ticketing platform, useful for the colleagues to get more context if needed;
- Description: explain why you have taken some decisions worths more than describing what you have done. That should be understandable reading the code; Sometimes a how section of the description is useful too;
- Regression Areas: 1 it’s not naturally understandable by reading the code; we have effectively noticed that’s useful to give context for both the review and testing phases because we cannot imagine that everyone has a full knowledge of the whole project;
- Milestone: assign your PR to a milestone that is related to one of the future releases, in this way generating the release note would be super quick!
- Checklist: this is the self-assessment phase. What I’ve started doing the last couple of years is to review my PRs before raising it and reading it as it’d be a PR from someone else. That’s an incredibly useful exercise which would help the whole team to have a smoother review process;
Lately, I started reviewing my PRs one second after I open them and before notifying my peers. It’s an helpful activity, I fix my own things before my colleagues check saving their time and also I’m definitely more able to identify logic errors (or to validate the logic also)😇
— Ennio Masi (@EnnioMa) September 20, 2018
- Labels: With the labels we can tell the colleagues which is the current status of the PR, we have In Progress, Review Needed, Ready for Testing, In Test, Work Needed, Testing Approved; The following chart shows the lifecycle of a PR. A PR, to be approved and move from Review Needed to Ready For Testing must receive at least 3 approvals;
Once the PR is raised and it’s in Review Needed state, the possible flows are:
- If the PR gets at least three successful reviews, it goes to Ready for Testing
- If there are comments to be addressed, it goes to In Progress
- Once the PR is moved to In Test, then it can go back to Work Needed if there are regressions or requirements not met, or it can go to Testing Approved which means the PR is going to be merged
Code review principles
For us the primary purpose of a code review is to assess the YNAP iOS code health and to make sure it improves over time. This is tricky because any evolving code tends to incrementally decrease in quality, that’s a fact.
So the goal of any contributor in our team is to feel comfortable contributing to the codebase, delivering features, refactoring code, etc. keeping in mind the team guidelines and trying to follow the project general architecture.
Each reviewer, on the other side, has responsibility for the reviewed code too and he/she should tend to approve a PR when it’s in a state which improves the codebase, even though it’s not a perfect PR. This doesn’t mean that any PR must be quickly approved and as a general rule the reviewer shouldn’t be afraid to request changes and the contributor shouldn’t take any denial as personal judgment.
“Criticism is almost never personal in a professional software engineering environment — it’s usually just part of the process of making a better product”. Fitzpatrick, Collins-Sussman: Debugging Teams, page 16
All the comments on PRs shouldn’t be driven by personal preferences but they should be risen to lead to a better codebase (in terms of both maintainability and testability).
In terms of coding polish and general PR health check we make sure that the code guidelines are followed making automatic checks on the PR via Danger:
General approach reviewing a PR
- Be humble!
- Be open!
- Never judge people personally, always refer to code and solutions (talk about the code, not the contributor)
- Respect and trust the contributor
- Comments should follow best practices, not personal style preferences
- Don’t leave open comments, suggest solutions instead
- Don’t put pressure on the colleagues! From the author perspective, it’s not fair to ask for any urgent review to the colleagues, a request like “Can you please approve my PR?” should be replaced by “Can you please review my PR?”
- Celebrate 🎉! If you see some code, a solution, an approach that you really like, let the contributor know! That always helps the team morale 🤗
If for any reason there is a code conflict in a PR, the best way to deal with it is for the contributor and the reviewer to have a face to face conversation. If it doesn’t help for any reason, we arrange a 30 minutes meeting with the whole team to talk about it.
During this year we have found some ways to make how we raise and manage our PRs which is effective for our team. I hope some of the suggestions can help other teams but I’m 100% sure that in the future the set of guidelines, general rules around the process could be completely changed. It’s definitely an evolutionary process that will never be perfect ☺️ I’m also sure, though, that we have to try to help the team to understand the foundation principles of the team itself that for us are:
Feel free to contact me on Twitter for any comment. I’d love to hear any kind of feedback! ✌🏼 And I hope that this article can help you and your teams!
Thanks for reading! 👽