Newsletter Archive

How do you review a Pull Request?

The first thing you should do is read the title and the description thoroughly. Your co-worker took her time and made sure you can get all the information you need out of the description. So read it carefully. Take a look at the screenshots and videos, so you get a sense of what it looks and feels like.

The next thing you should do? Check out the branch locally and run the tests. If the PR isn’t WIP anymore, the tests should all be green. On the CI server as well as on your machine. It is important to run the tests on your computer as well, because you could have a different setup with your database that could surface a hidden issue. Or you are using a different browser than your co-worker so you might see certain problems that are browser-specific. Looking at the feature on your computer provides the benefit of becoming more intimate with the changes and being able to look at them in a bigger context. The review interface on GitHub and GitLab often only show the changes and small amounts or neighouring lines. On your computer, you can see the whole glory. This is good for context.

When you find things that you want to comment on, please do this kindly. Be generous, everybody makes mistakes — even you. Write your comments as questions. This could look something like:

> I noticed you instantiate the Foo class inside the constructor. What were your reasons for doing that?

Even if you already know, that dependency injection would have been a better choice for this spot, help the other developer figure it out by asking questions. Do not tell them what to do or to change. Show them the direction, point them to places they could learn about how they could do it better, or explain why you think something might make more sense if they do it differently. The more context you provide and the more care you take to explain your questions and suggestions, the easier it will be to improve the code.

Remember: [You are not your code].

Things to look for when reviewing a PR

There things that your linter won’t catch. These are long or unclear variable or function names. There might be code that is commented and can be deleted. Perhaps it was there from an earlier version of the code, as a draft. You could even find violations of the style guide, that the linter did not catch.

Perhaps your team writes documentation, like in classic docs or in a wiki. Was it updated correctly? Can you understand what was written? Is the ticket’s status updated and are all references to other issues or related changes present? Perhaps you can point the author to other changes in the code, or features or resources that are related to the thing you are reviewing (mailing-list threads, help-desk/service tickets, blog posts, articles or books)?

Take a look at the commits that are part of the PR. Do they conform to your commit-message guidelines? Do they make the feature’s history clear? What do you learn from scanning them? Are there any changes necessary?

Let me state again that the most important thing is to be polite, helpful and respectful of your co-workers. PRs should be a place to improve (the code and yourself) and to learn.

What role does CI/CD play in your PR?

My guideline for development workflows leans heavily on having a CI/CD server that orchestrates the whole workflow. CI/CD means continuous integration and continuous deployment. There is a lot to CI, and why and how you should do it. Look for multiple articles on that from me here. For the time being, you can get a wonderful introduction to the subject from another expert: [Martin Fowler on his site].

Your CI server should do certain steps when a PR is opened. These include:

  • Check the code for linting errors and warnings.
  • Run all automated tests, including long-running integration tests.
  • Run static (and behavioral) code quality analysis.
  • Deploy the feature branch to a test environment automatically.
  • Inform the reviewers about the results of the above steps, through a comment on the PR, and post the url of the deployed test environment.
  • Block or unblock the the PR from being able to be merged, depending on the results of the above steps.

If you make your CI server report all linting errors, you free your developers to focus on the changes in the logic. Imagine the case where reviewers wants to check your PR, and the first thing they have to do is create lots and lots of comments about linting violations. Nobody will be able to focus on the really important changes and the discussions will grow very big. So let your robots (CI server) handle that for you.

Refactoring without a care

Before I get to today’s topic, I would like to say thank you, to you.

My little poem yesterday seemed to resonate with you. At first, I planned to write about it and its meaning today. But your responses indicated that it spoke to you. And I wouldn’t want to ruin this with my ramblings about it. So I’ll just finish with: I enjoyed this very much.

Lately, I spent some more time on Twitter. I don’t know how to use Twitter well (enough). I always have trouble with creating threads or topics. But I (re)discovered some very interesting people, sharing their ideas in long threads.

A few days ago I came across @GeePaw Hill. I believe it was because I followed a few tweets by Kent Beck. GeePaw Hill had this thread where he encouraged people to refactor without caring for the application domain, only for the code. You can read the thread here.

He even elaborated some more on his blog.

I find the idea fascinating and will continue to think about that.

Computer says “no”

[…] we do have formal rules that we should obey when writing code. A team has rules, and new team members need to learn them before trying to write any code.

That’s what I wrote yesterday. It’s my email so I can write whatever I think is correct. You’ll let me know through your answers if I am wrong.

My friend Tino answered on Friday and asked whether a university degree or certificates might function as a driver’s license. And that is true to a certain degree. I am getting a new certificate these days as well. I hope to complete the exam on Wednesday (ISTQB Advanced Level — Technical Test Analyst).

The obvious difference to a driver’s license? I am not legally required to obtain one before I can start writing code. Tino also said that he’d find it interesting to be (self)tested in current web-standards and best practices. I do believe these tests are valuable. If I come around to create one, I’ll let you know.

Back to the beginning of the email. Why do rules matter to a team? Developers have their style for writing code. Even if there are rules and certain regulations you have to follow, developers still find ways to write code in their unique style. And that’s a good thing. It would be boring otherwise.

Still, this style has to obey the rules. Here’s why:

  • The code won’t be too complex. Because your static analysis tools tell you if your cyclomatic complexity metric is too high.
  • Classes and modules will have low coupling and high cohesion. This leads to code that’s more easily testable and has higher reusability than other code.
  • If you have an error if explaining comments are missing, you could make sure that your developers take some extra time to make sure code can be easier to understand. Other rules, like variable and method/class/module naming conventions, have the same goal.

In short: Rules help your team to write code that is maintainable and has low technical debt. This reduces the total costs of ownership. If you only look at the cost of writing the code and delivering the software, the costs might be higher if you follow stricter rules. Over the complete lifecycle of a software (product), the total costs would be lower because of better maintainability and a lower number of defects.

This is already getting long. See you tomorrow with even more thoughts on this topic.

Driving on the left side of the road

We don’t have rules of the road for software development. You don’t have to stop at every red light or keep your speed below a specific limit.

Well, yes. We do have rules. If you write your whole program in only one file, someone will tell you that this is bad. At least I hope that’s the case!

If you only use variable names like x or y, your coworkers will flag this during code review. Perhaps you already have static analysis tools that tell you before your coworkers do?

While we do not have a driver’s license, we do have formal rules that we should obey when writing code. A team has rules, and new team members need to learn them before trying to write any code. Otherwise, it could feel like driving on the wrong side of the road: Driving on the right side of the road feels natural to you if you’ve never done it any differently. But it can have dramatic consequences if everyone else expects you to drive on the left side.

Coding license

Every country I know has an obligatory driving license before you are allowed to drive a car by yourself.

No country I know has an obligatory coding license before you are allowed to code by yourself.

The longer the time that passed between the driving license and the current day, the more reckless and careless drivers have become. What I mean is that new drivers are careful. For the rules, for their passengers, for other cars and people. The longer they drive, the more they bend the rules, pass yellow or red lights, or speed just a tiny bit. Everonye does it, why shouldn’t they do it as well. No one is looking anyway…

I won’t argue for a coding license. It would be fruitless anyway, and it would be too hard to establish a standard. But there so many simple (not easy) wins you could have with the proper knowledge and attitude. So many legacy systems less and so many more successful projects.

When was the last time you compared your skills in coding with other people and had the possibility to spot places you could improve? How do you score your ability anyway? How do you decide what to learn or focus on?

Do you decide these things before you start to code on a project, or do you only find out in hindsight, when issues arise or new features begin to get harder to realize? Do keep a list of problems you identified and make sure you avoid them in the future?

I’ll take the next week to look into this more.

Software rewrites – Does it make sense?

I came across a very interesting article written by Herb Caudill, on different perspectives on rewriting software. He highlights 6 different stories of how a rewrite went. You can read about Basecamp, Netscape Navigator, Gmail, and others.

It is a long article, over 30 minutes according to Medium.com. And I am also sorry for linking to Medium. I don’t like them, and I resent sending them traffic. But this story might be worth it.

Here you go: Lessons from 6 software rewrite stories

NB: I do have experience with software rewrites myself. I took part in two endeavors. One thing was a client application. The legacy app was written in Rails 2 (I believe, it might have been Rails 3) and heavily patched. This made maintenance and feature development quite expensive. We rewrote the software but kept quite close to the original in functionality. The rewrite enabled us to use modern gems and solutions we had created in-house for other clients. This used synergies. It was an ambitious project. In the end, I think it didn’t make too much sense, financially. But I couldn’t be sure about that one.

Another project where I helped on a rewrite concerned frameworks for iOS applications. We had customers that wanted to publish iPad magazines on the App Store. To make it easier for themselves, previous developers had written a custom publishing framework. This framework was reused on every project. It was extendable, reusable and efficient. But it was also difficult to handle and very limiting with regards to the layout and design of the magazines. Which was a problem for the clients. So they set out to rewrite this. I joined the company while the project was still in progress. I left 2 years later. The project was still ongoing. The rewritten framework was used in every client project, alongside the older framework. For some features, you had to use the old one, because they weren’t yet supported on the new framework. For other use-cases, you had to use the new framework. Especially for certain pages in the magazine, with new layouts. Yeah, it sucked.

Answering a comment about “Delete all your tests”

Matthias Berth is a German expert on software delivery and software quality. He politely disagreed with me on the idea that you should delete all your tests.

I decided to call this day “Video Wednesday” and record an answer as a video. I just posted it on LinkedIn, and thought you might like to watch it there.

It even has subtitles 😉

Motivation, or: How to get your coworkers to write better code

How can I get my coworkers to write better code?

We closed with this question, yesterday. If you want to be able to motivate your coworkers to write better code, you have to know where they stand right now. I already wrote a few articles onthis topic. Follow these links, and you’ll get a good idea on what to measure, how and why. You’re welcome.

Continue reading Motivation, or: How to get your coworkers to write better code