Category Archives: Process

10 Rules For Working Successfully With Pull Requests

“Don’t expect to build up the weak by pulling down the strong.”
— Calvin Coolidge

Software development has always been a rich source of religious wars: emacs vs. vi, tabs vs. spaces, brace placement. Where do I stand regarding “pull requests” vs. “trunk-based” development? Even though it looks like the really hip developers opt for “trunk-based” these days, I’m clearly on the side of “pull requests”. But only if certain rules are followed.

Rule 1: Keep pull requests short

Don’t work on a feature branch for days. That’s against the idea of continuous integration. Not only do you risk integration problems, people will have a hard time reviewing your large changeset. Instead, split-up the pull request into several small(er) pull requests, for instance: preparatory refactoring, main implementation, clean-up, additional unit tests to improve code coverage. A pull request comprising 50 changed lines is easy to review, a pull request comprising 1000 changed lines, hard. What is easy to do gets done, what is hard to do usually gets delayed.

Rule 2: Constantly collect ideas on your way

Resist the temptation to cram more into your pull request than necessary. If you discover a badly named class or a way-too-long function, don’t clean it up right now. Make a note of it, add a TODO, or, even better, create a technical debt ticket. This keeps your pull requests short (see rule 1) and provides you and your teammates with work while waiting for feedback (rule 3). When implementing, review existing code and always be on the lookout for things to improve. Collect ideas for hard times like a squirrel collects nuts.

Rule 3: Be patient while waiting for feedback

Most developers, after having published their pull requests, only have one thing on their mind: integrating it asap. If they haven’t got any feedback or approval within two hours, they start hassling teammates, complain that they are blocked, do all sorts of whining. Such developers need to realize that while their changes mean all the world to them, they most likely don’t mean much to others. What developers should really have on their mind after publishing a pull request is this: a) are there any open pull requests from others that I could review? b) are there any technical debt tickets that I could tackle? If developers follow rule 1 and rule 2, there’s little chance that they ever run out of work.

Rule 4: Be kind and grateful when giving and receiving feedback

Nobody likes being criticized but every criticism should be viewed as an opportunity to learn for all involved parties. When you give criticism, be constructive and avoid cynical and arrogant remarks like “Only rookies would use a for loop here”. If you get harsh and unfair criticism, don’t take it personally and don’t add to the fight. Calm is contagious.

Rule 5: The author has the final judgment

I once was part of a team where people had a pull request battle over what’s the best way to initialize a string constant in C++. “Proof of efficiency” was shown by contestants in the form of disassembly for various platforms and they all showed different results. This ridiculous battle went on for days. Let’s not forget this: everybody is welcome to criticize the author’s code, but in the end, it’s the author’s decision what to accept and what not. The pull request doesn’t have to satisfy everyone and a software project is not a democracy. A good way to end a fight is this: “Thanks for your feedback, but I’ve decided to keep the code as it is”.

Rule 6: Advertise your pull request

If you want to make sure that nobody looks at your pull request, use a meaningless title full of spelling mistakes like “REwork for prformace”. If not, craft the title carefully to express it’s purpose and importance. I’m sure that the same pull request with this title will be reviewed much sooner: “Fix: Use a faster scanning algorithm to prevent nightly build timeouts”. The same goes for the description of the pull request. Give a crisp executive summary of the motivation and what you are trying to achieve. Don’t give long sermons to avoid TL;DR.

Rule 7: Review your own pull request

Before publishing your pull request, carefully review it yourself. It’s amazing what you can detect yourself, especially silly goof-ups and typos. Make sure that the pull requests is of highest quality, to the best of your knowledge. Don’t waste your reviewer’s precious time or they will soon start wasting yours by not looking at your pull requests anymore.

Rule 8: Publish only when you’re done

There’s nothing more annoying for a reviewer than having to constantly re-review because the author keeps on pushing changes. If you find out that your code needs rework, set your pull request to draft state. Publish it again once you’re done.

Rule 9: Proactively explain unusual changes

Experienced developers know that code shall be written such that it is self-explanatory and comments should be only used to document surprises. Still, certain changes to obvious, non-commented code might still be surprising to reviewers. In such cases, add an explanatory review comment yourself such that reviewers don’t have to ask you about it.

Rule 10: Spread the word

If teammates unknowingly violate these rules, educate them about these rules. If they don’t care or continue to deliberately violate these rules, let them wait next time they want you to review their pull request. Show, don’t tell.

A Neverending Story

“Nothing is lost. Everything is transformed.”
― Michael Ende, The Neverending Story

I explained in this post that I don’t view technical debt as something that is bad per se. Rather, I believe that “good technical debt” — at times — should be employed for strategic reasons. As an example, when developing a feature, you might take shortcuts in order to unblock stalling teammates who need your changes in order to carry on with their own work.

Let me reiterate: to qualify as good technical debt, it must be

1. taken on consciously
2. managed
3. repaid timely

How can this be achieved in practice?

Once you’ve made a deliberate decision and considered the pros and cons, you implement your makeshift solution. But before you mark your task as done, you create another task which has the goal of removing the technical debt you just introduced. But where should you put this task?

Don’t hide it in the product backlog as it a) contains usually many issues already b) is more concerned with externally observable features and c) is actually owned by the product owner. If you did, most likely technical debt issues would be delayed (or rather ignored) in favor of “real” stories, which would grossly violate good technical debt requirement #3: “repaid timely”. Instead, put it under a story of the current sprint titled “Repay technical debt”.

An immediate advantage of this approach is that it makes your shortcuts visible to the whole team. Further, everybody, including you, can pick up this task and just start working on it. However, in typical cases, such clean-up tasks won’t be done in the current sprint. So what happens with the “repay” story and all attached tasks at the end of a sprint?

You move it to the next sprint, of course! Consider it a story that never ends, which nicely mirrors a well-known software engineering truth: the fight against software entropy goes on forever. Since the story doesn’t go away, it’s a great reminder for the whole team that repaying technical debt (or constant improving of internal software quality) is of super-high importance.

Having this story in place allows you to manage technical debt, as stipulated by good technical debt requirement #2. There are too many technical debt tasks in this story? Maybe it’s time for a “technical debt sprint” where everyone focuses on getting the list shorter instead of adding new functionality. Are there no technical debt tasks at all? Maybe the team isn’t really tracking their technical debt. Another possibility is that the team doesn’t use “good technical debt” as a strategic tool and instead make other people wait for their gold-plated implementation.

In my view, a neverending “payback technical debt” story is a great tool. It’s a quality backlog maintained by developers which puts dirt right in front of everybody’s noses. I believe that this drastically mitigates the risk of creating a maintenance nightmare while still allowing for occasional shortcuts.

Technical Debt Is Not Bad!

“There once was a Master Programmer who wrote unstructured programs. A novice programmer, seeking to imitate him, also began to write unstructured programs. When the novice asked the Master to evaluate his progress, the Master criticized him for writing unstructured programs, saying, “What is appropriate for the Master is not appropriate for the novice. You must understand Tao before transcending structure.”
The Tao of Programming, Book 3, Verse 2.

I consider Ward Cunningham’s “technical debt” analogy as one of the most important metaphors in software development. According to him, he invented it to explain to his manager why his team needed to do refactorings: like financial debt, technical debt tends to grow over time and the burden gets bigger and bigger due to compound interest.

During the last decade, quality-minded developers have done a great job of educating their managers about technical debt but it seems to me that there is now the notion that all technical debt is bad and must be avoided. But Ward Cunningham never said such a thing — and rightly so.

In business, not all debt is considered bad. It’s a huge difference if someone takes out a 200k loan to buy a flashy sports car or if the owner of a construction company takes out the same 200k loan to buy a bulldozer. The sports car is a pure liability that makes the owner poorer whereas the bulldozer is an investment that will generate money in the future.

The same holds for technical debt. If you initially implement a limited version of a feature (e. g. slow, bloated, lacking proper error-handling) just to enable other teammates to carry on with their own work, that’s good technical debt.

So when does technical debt qualify as good technical debt? To me, good technical debt is

1. taken on consciously
2. managed
3. repaid timely

By contrast, bad technical debt arises out of laziness or incompetence, it lurks in the code base and won’t be repaid. Rather, like credit card debt in real life, more debt is accumulated over time and new loans are taken out just to repay the interest rates of other loans. This is the kind of technical debt that is to be avoided in the first place.

You can use a knife to kill somebody, but you can also use a knife to whittle and prepare food. Surgeons even use knives to save people’s lives. Thus, a knife is neither good nor bad per se and so is technical debt.

Yet Another Software Metaphor: Pitoning

“Gravity wins over all other known forces”
— Andrea M. Ghez

As software developers, we have to deal with conflicting goals on a daily basis. One example is finding an answer to the question whether to hack out new features or clean up existing code. Or, in slightly more professional words: shall we make progress in terms of functionality or in terms of quality?

THE PROBLEM

Non-technical/business people usually don’t understand the importance of software quality and tend to err on the side of rapid feature development—they assume that by focusing on features, they can sell their product sooner. Software developers by and large do understand the importance of software quality but usually also err on the side of feature development. Why? Because it’s much more fun to work on something new than doing mundane quality improvements.

As a result, many projects focus almost exclusively on churning out features. What these teams often fail to realize is that not only are features progressing rapidly—the overall risk is progressing rapidly as well.

In projects that opt for such a “code-and-fix” model, software entropy (some prefer the term technical debt) will constantly increase and even if these projects get finished one day (which is really not that certain) the messy code base might not be strong enough to support the features required for the next major release. Even worse, nobody is able to understand and maintain the messy code apart from the original authors, who have probably left the company in the meantime, anyway.

SOFTWARE METAPHORS

Software quality is yet another term for investment protection. It ensures the money that went into a project is properly insured. For all but the small, short-lived projects such insurance is crucial for the long-term success of any company.

Metaphors and analogies like “investment protection” and “technical debt” have quite a history in software development. For decades we have talked about “building software,” “software architecture,” “bugs,” and rightly so: by comparing something we are not familiar with to something we understand well, we can gain insights that help us understand the unfamiliar topic.

PITONING

Recently, I had an idea for yet another software-related metaphor, which I call “Pitoning.” Pitoning compares software development to rock climbing, and it helps in finding trade-offs between feature progress and quality.

With the exception of free solo climbers, rock climbers don’t rush to the summit without breaks. Doing so would be extremely fast, but also highly risky—even a minor slip could be fatal. Instead, after they have made some progress, sane rock climbers will drive a piton into the rock and hook into it before climbing on. This “pitoning step” costs some time, of course, but it increases the probability that climbers will make it safely to the top significantly.

When we set out to develop a feature, we typically don’t know what’s the best design, so we experiment and try out different approaches. During this exploratory phase, we don’t want to waste time making the code as beautiful as possible, since it might be replaced by something else, eventually. However, once we have a working solution, we do the pitoning; that is, we add missing unit tests, we refactor, we make the code robust and in general industrial-strength, which may sometimes even include drawing UML diagrams and writing documentation.

CONCLUSION

The key concept behind pitoning is that a) experimentation and learning are not just good but essential and b) cleanup is done immediately after a working solution has been implemented—not at “some” point in the future. In certain cases, the pitoning phase will be longer than the exploratory phase. But skipping pitoning is not an option—gravity will surely get us. Many products and even whole companies have gone bust because of product recalls and lost reputation.