Left On The Web

On Code Reviews

| Comments

Recently we’ve had some discussion on the PHPNL Slack community about code reviews. It’s an interesting topic that I’ve been meaning to blog about for quite a while already (and I’ve written about it for Wisdom of the elePHPant), so this was a good reason to sit down and actually put down my thoughts on code reviewing.

So what is it?

Code reviewing is exactly what it sounds like: It is reviewing code written by another developer. There are different ways of doing this, but in the end it all comes down to having at least one other set of eyes checking any code written before it is released.

There’s many reasons for doing code reviews. It can be to prevent security issues, to ensure correct performance of your application, to prevent bugs but eventually it all comes down to the more generic term of ensuring the quality of your application.

How do you do code reviewing?

There’s many tools that will help you with code reviewing, and I can’t give you The Perfect Tool For The Job. What currently works well for many people is using the approach with Git where you send a pull request. Most Git-tools (such as Github and Stash) have excellent interfaces for going through the diff between a feature/bugfix branch and the main branch. They allow you to add comments with every line of code, so that the author of the code knows exactly what you’re commenting on and is able to fix the right code easily. Similar tools also exist for many of the other version control systems out there.

While tools can make your job easier, it is not necessary to use them. You can go through a diff when using your version control system from the commandline, or you can even go through the code together with the author of the code while sitting next to eachother (or while using screensharing with Skype/Hangout).

Preparing the review

As a developer I always make every change (feature, bugfix, improvement) in a seperate Git branch. The branch is pushed to my fork of the main repository, so once I’m done with my change I push the branch to my fork. I then create a pull request against the main repository. The PR description should contain useful information: A link to the (Jira) issue this is a PR for and possibly some generic information on what you’ve done in this specific PR and what choices or assumptions were made.

When the PR is created, I immediately review it myself. Reading a diff on Github or Stash is completely different from working in your IDE, is my experience. I look at my code in a different way and often I spot small bugs, forgotten changes or useless changes in this mode that I don’t when actually working on the code. I often also spot forgotten docblocks and such pretty quickly when reviewing my own code.

Doing the review

When I sit down to do a code review of code written by someone else I open up the pull request I’m about to review. I click the link to the Jira issue to check what the code is supposed to do and then start looking at the code. I focus on many things: Does this logic indeed implement the feature requested, but also: Does it follow the standards. I am known for nitpicking over docblocks, typehints and such.

Depending on the testing strategy in your organization, this is also the time to check if all code has accompanying (unit) tests, and whether they are actually good tests. Testing is a subject on its own of course, but it should be part of your review to check if the tests are there and good.

For everything I find, I place a comment. Even when there’s several places in a file or PR that have the same “issue”, I place a comment in each place. I’ve gotten some feedback that some people don’t like this and prefer “check this PR for x, there’s several places where this occurs”. This is a personal preference, but I prefer getting the comment in every place where it’s wrong, because when running through the feedback for a pull request it’s easier to know exactly what you should update where.

When commenting, pay attention to the way you write your comment. Code reviews are a check of the code, not the developer. Comments for that reason are not an attack on the person, but should be directed at the code. Explain why something is, according to you, not the right implementation, why you’re commenting, why you think it should change. Always keep in mind that there is a human being on the other side of the pull request that needs to handle the comments.

When code and/or documentation isn’t clear: Ask questions! You can ask the developer directly but you can also add a comment with the question. Reviewing is not something you do once per pull request and then forget about, a pull request can actually contain whole discussions and conversations. It is wise though to keep this to a minimum and when it turns out there’s discussions/conversations going on, try doing this directly and put the conclusion in a comment so that the pull request does contain the information that is important to decide whether the pull request can be merged.

Functional review

While working at Enrise for a project some time ago, I got introduced to the concept of also doing a functional review of a pull request. Every pull request should be reviewed by two developers other than the author, and one of the developers is also supposed to do a functional test of the pull request. This means you don’t just check the quality of the code, but also the functionality. I really like this approach. Code is only code, it doesn’t turn into functionality until you actually execute it.

Processing the comments

Back to looking at the review from the developers perspective: Once your pull request is reviewed, it is up to you to process the comments. This definitely doesn’t mean you actually have to do exactly what is said in the comments: Look at every comment and see if you agree. If you agree, implement a change to process the comment. Otherwise, respond to it to state your position on the subject. As mentioned above, when it looks like it is going to turn into a discussion, it is better to not do that in the pull request but instead do a direct conversation and only document the conclusion of that. When you decide not to follow up on a comment, make sure to explain why.

As mentioned before, a code review is not a “check once then it is fine” thing. Instead, it is a play between all participants in the code review that goes on until the pull request is approved. So when you’ve made your changes to your pull request, you send it back for review and another review may happen.

Approval

Depending on the organization you work for and its strategy in these things, once the reviewer(s) approve of the pull request it may be merged immediately or will be merged during the preparations of a release. Approval is usually the moment where you’re done, your (Jira) issue can be moved to the next step in the process and you’re done. Congratulations, you just contributed more good code to the project!

Who reviews?

I personally think that in a team of multiple developers, everyone reviews. Not just the seniors should review the code. Depending on team size, you’ll make a decision on how many people should review a specific pull request though. When you have a small team, you’ll probably want one other developer to review, but when you have a bigger team it’s a good idea to have at least two people review each pull request. This should give a more balanced review than just a single developer.

Seniority should play no role on deciding who reviews. Senior developers usually have a pretty strong opinion on how things should be done, but are sometimes also stuck in certain patterns. It can be refreshing to have a junior or medior do code reviews because they may comment on things that seniors don’t pay attention to or that they don’t see. Aside from that, code reviewing can be a great way for a junior or medior developer to see how other developers approach certain problems and implement certain features. The learning goes both ways!

Group reviews

Aside from the system where each pull request is reviewed before it is merged, it can also be a good idea to do group reviews every once in a while. You’ll need a laptop, a beamer and your codebase. One person controls the laptop and as a group you go through your codebase. Everyone is allowed to speak up when they see something weird in the codebase, which then causes a discussion (well, usually ;) ) that can result in an improvement ticket in your issue tracking system.

In group reviews it is just as important to realize that comments and discussions are not critizing the author of the code. It is an effort to improve the quality of the codebase. As such, don’t offend and do not take offense from anything discussed during a group review.

Code reviews when alone

Doing code reviews when you’re a freelancer working on your own can be hard, but not impossible. There are many ways of doing this. The easiest is to find a group of likeminded freelancers that are willing to exchange favors. Those favors can include doing code reviews for each others projects. The feedback that another developer can give you is invaluable and can really lift your codebase to a higher level. Especially when working alone, this can be very important for your personal development and for your code quality.

Code reviews in the QA ecosystem

It is important to realize that code reviews are part of the QA ecosystem. As such, they can not replace other parts of that same ecosystem. You can not replace code reviews by a setup with PHPMD, unit tests, PHPCPD etcetera as you can not replace those tools just by code reviews. They should be done side by side to get the best out of all those tools.

Solving ‘Convert: Delegate Library Support Not Built-in `white’ (LCMS) @ warning/profile.c/ProfileImage/853’

| Comments

Another quick fix documented here simple because it may help me (or someone else of course) in the future.

Context

For a project I’m working on right now I need to crop an image using ImageMagick. We’re using the commandline instead of Imagick because we need to execute a custom command with a lot of extra options for this specific action, since the image will eventually be used for high-quality printing. As I was testing the command, tweaking the parameters, I eventually ended up with an error:

convert: delegate library support not built-in `white’ (LCMS) @ warning/profile.c/ProfileImage/853

The solution

It turns out this was very simply to solve: I used homebrew to install imagemagick, and I installed it simply by doing:

brew install imagemagick

Apparently, this is wrong. At least when you want to specify your own profile (for instance by using -profile /path/to/sRGB_IEC61966-2-1_black_scaled.icc). The solution? Install it —with-little-cms:

brew install imagemagick —with-little-cms

With thanks to clee704 on Github who came up with this solution.

SSL, Composer and PHP 5.6

| Comments

A quick technical note: I ran into the same issue as Rob Allen did. Using his solution I came to understand the problem, but contrary to Rob, I was using an Ubuntu system in a Vagrant-box.

The solution is the same, the path is different. In your php.ini, you add/update the openssl.cafile parameter:

openssl.cafile=/etc/ssl/certs/ca-certificates.crt

Don’t forget to also restart Zend Server if you’re going to use this through the webserver as well. Thanks Rob for your blogpost and Cees-Jan for finding the right path to this file.

Coming Out of the Box

| Comments

Asking for help. Once you start thinking about it, it is really hard to do. It doesn’t really matter what you need help with, it is hard once you start wondering whether you should ask.

In The Art of Asking Amanda Palmer writes about Anthony. Anthony is a professional therapist who helps people in hard times. Anthony is also Palmer’s best friend. He helps her a lot, either by listening, by asking questions or by giving advice. But once Anthony himself is in a dire predicament, all of a sudden, it was hard for him. She writes about this:

He likes being in control, he loves having answers, he loves fixing and helping people. But he has a really hard time letting people help him. Sometimes, when he gets depressed, he shuts down and doesn’t like to talk. When that happens, I figure it’s time for me to step up, ask him questions, help him through, talk about the problems. Be he clams up and doesn’t like talking to anyone about his own problems. He calls it Going Into The Box.

Triggered by both this book and events in my own life, I’ve been thinking a lot about “asking”, and have been thinking about how I handle asking, and I’ve come to an interesting conclusion. Asking is easy if you just do it. Once you start thinking about it, it becomes hard.

I really recognize myself in Amanda Palmer’s description of Anthony. I, too, have (had?) a tendency to completely turn inward once I have a hard time. The trigger can be anything from worries over something to stress, but not even my wife will be able to get through to me in such a situation. Why? Because I don’t want to ask for help. I don’t want to be a nuisance to other people, don’t want to bother them with my own problems. Recently though, I’ve started talking about it to my wife, asked her for help coping with something, and it turns out that once I do that, the problem gets much easier to handle. I can cope.

On the other hand, when you don’t really think about it, asking is easy. In the preparations of WeCamp, we were so excited about our concept and the way things were shaping up that without much hesitation I sent some people an e-mail asking them to be a coach at the event. Being a coach would basically mean “being on an island working for a week, and not getting paid for it”. But I didn’t think before asking, I just did it. And the response was excellent. Not everyone I approached could to it, for a variety of reasons, but I did not have a hard time asking. Why? Because I did not think about it before asking. I just sent out the e-mails and did it. Because I was really excited about what we were trying to do.

I’m trying to train myself to ask more. Sometimes, this may be annoying but I’ve found asking works. If people do not have the time to help or even answer your question, they’ll just let you know. Most people actually love to help once they’re asked. I know I do, which makes it even stranger that I didn’t understand that asking would probably result in the same help I give when someone asks me.

Try to be like Anthony when it comes to helping. Try to avoid being like Anthony when it comes to asking help. Come out of that box.

We Are All Artists

| Comments

I’ve written on this topic before, but as Amanda Palmer touched the topic in her book I felt it was a good idea to write about it again, this time on my blog. Especially since the angle is slightly different.

In my contribution to Stuart Herbert’s book I talk about the need to be proud of your work with the angle of ensuring a certain quality. However, there is also the other way of looking at it. I’ve talked to a lot of people who at some point in their career (often early on in their career) had a hard time being proud of their work because their work can not be seen. Designers and frontend developers can very easily show their work. “Hey, look at this cool site I’ve designed!” For backend developers, this is very hard. “Look at this site, we’ve worked hard for 3 months building it!” “It’s just a webshop, so what?

In her book, Amanda Palmer speaks about a phonecall she had with her mother who had been a computer programmer for 40 years. Amanda Palmer, being a pretty clear and proven artist, at some point gets confronted by her mother with something she said when she was 13:

MOM, I’m a REAL ARTIST. You’re NOT

Not long after, however, her mother says exactly the thing I’ve wanted to say to many of the developers I mentioned above that didn’t know how to take pride from their work:

You know, Amanda, it always bothered me. You can’t see my art, but… I’m one of the best artists I know. It’s just… nobody could ever see the beautiful things I made. Because you couldn’t hang them in a gallery.

This is the message I’ve tried to get across to many developers in the past years. Sure, you can’t show non-programmers your work of art. Even if people can appreciate the amount of effort that goes into building software, many a mother, brother, nephew or neighbour would not be able to understand the difference between your building a high-end high-performance architecture and someone that “just makes it work”. But that doesn’t mean you stop being an artist. YOU ARE AN ARTIST. You need to solve your problems, implement your features, you need to be creative. Being a software programmer is creative, just as being a musician is creative. It’s just a different problem you’re solving.

So please, PLEASE be proud of what you do. We are all artists.

Update: Bart de Boer pointed me to a 1995 interview with Steve Jobs in which he mentioned the exact same thing. If even Steve says so, do you believe it now? ;)

UWYC: Use What You Can

| Comments

In her book, Amanda Palmer talks about DIY, and how when you start asking for help, Do It Yourself is a strange term. Instead, she suggests UWYC, which stands for Use What You Can. I think software developers can learn a lot from this mindset. As Palmer says:

I have no interest in Doing It Myself.

This is exactly how we should approach software development.

Many of the problems we try to solve on a daily basis, whether we work on projects for clients or we work on our own products, have been solved before in one way or another. A lot of the solutions to these problems have been posted as open source packages on, for instance, Github. Especially with Composer and Packagist it is now incredibly easy to find these solutions. So why would you not use these solutions? Even if they are not a 100% fit, there is a good chance that a big part of the solution fits your problem.

Palmer says:

There’s really no honor in proving that you can carry the entire load on your own shoulders. And… it’s lonely

This is an important lesson to us developers. Trying to solve every problem yourself is a humongous burden for you as a developer. No matter how small the problem is, do not underestimate the effect this may have on you and your codebase. And… it’s lonely. If you have the NIH-syndrome and you’re looking for new developers, there’s a very good chance there’s not a lot of developers interested in working with you. Developers usually want a challenge, but they want a good challenge. They want to solve the problems unique to your project, not the problems they’ve already solved many times before.

Filter the solutions

Obviously, not all open source software and libraries are good. Everyone can publish code into the open source world, so you need to filter the solutions you found into usable and unusable solutions. Don’t make this a final thing: The solution you find inadequate today may prove to be really useful in a year’s time, when either the code author has been working long and hard on the code or your problem may be slightly different from the one you’re solving today. Always filter for the problem at hand, with today’s code.

Some things to watch for when filtering:

  • Use of best practices (does it support Composer, use external libraries, adhere to PSR-standards)
  • Has the code been documented well?
  • Is the code under active development?
  • When it supports Composer, how many installs does it have according to Packagist?

This list can be huge, and not all of the above need to be answered positively for a solution to be a good solution. Have a serious look at the code itself to see if it solves the problem. Eventually, the most important thing to filter for is does this code solve my problem?

This goes beyond libraries

The most obvious subject of the above is code: The libraries and components you use in your project(s). However, this goes beyond that. Any tool you use, ranging from your IDE to your version control system, and from your Continuous Integration system to … yes, even the programming language you use. All of it can be evaluated in the same way as I’ve discussed above.

The Art of Asking

| Comments

With Ingewikkeld we’ve sent out a gift to some customers and other contacts. A book that has nothing to do with PHP. While this may seem odd, it definitely isn’t. What book? The Art Of Asking by Amanda Palmer.

Amanda Palmer? Who is she?

Amanda Palmer is an artist. Performance artist, musician, cabaret punker, living statue, author, blogger and above all human being. Along the way she has done some things and while doing those things she’s learned a lot. And in her book The Art Of Asking, she has shared a lot of the lessons she learned. On life, music, community, crowdfunding and many other things.

But… what does this have to do with PHP?

Nothing. Well, also everything. Believe it or not, we all have lifes. We all operate within a community. Some of us work a lot with crowdfunding. Most of us love music and a lot of us need music to code. And really, our work is very creative. Not in a musical or graphical way, but we need to find creative solutions to often complex problems. In a way, we could be considered artists.

So what about this book?

The Art Of Asking is a book that is not just about asking. It is about life lessons. It is about life. It is about understanding. About human relationships. About experiences, good and bad. And while in a totally different community, it is about many of the problems and challenges we also face in the PHP community today. I think we as a community can learn a lot from this book.

What can we learn?

There’s too much to put into a single blogpost, so over the coming weeks I’ll be posting a series of blogposts going into more detail on the lessons we can learn from the book and why I think it also applies to the PHP community. In the meanwhile, feel free to also follow Amanda Palmer on Twitter or read her blog.

The articles:

UWYC: Use What You Can We Are All Artists Coming Out Of The Box

The Inter-team Standup

| Comments

For WeCamp 2014, we were creating multiple teams that would all work on their own project. But since we wanted to maximize the exposure of all delegates to the lessons learned by all teams, we needed to create a moment where people could exchange their ideas, their lessons, their work.

Since our schedule already contained a 15-minute standup meeting for each team, it was not hard to come up with the idea of organizing a central stand-up meeting with all teams.

Our Inter-team standup was simple: All members of all teams would be meeting together. Each team would appoint a single representative that would tell everyone what the team had been working on, what challenges they encountered and what solutions they’d chosen. For us, not just the what was important but also the why of decisions. Since we want people to not just learn about the approach but also why this approach was taken, we tried to put some focus on that as well.

Taking this outside the camp

Recently at a customer, we were having a discussion about how to improve the communication between the different development teams. Both teams work (partially) on the same codebase, each with their own responsibility. While doing their work, they may (try to) solve the same problems. That seems inefficient and may result in inconsistent solutions to the same problem.

During this discussion I thought back of WeCamp and proposed we would try this approach. This week we had our first standup, which was quite interesting. It’s good to hear, in just a couple of minutes, what the other team is working on and what problems they are solving. We did notice that the meetup needs a bit more preparation from the teams to make it more efficient, but the concept of having a (in our case) weekly meetup of 10-15 minutes seems to work well.

Don’t replace normal communications

It is important to realize that this meeting is not meant to replace all other forms of communication between teams. Aside from this, it is important that individual team members can visit the other team when they have a question, that they have Skype/Slack/HipChat/IRC to communicate, that they talk to eachother by the watercooler or coffeemachine and that they go to lunch together. The inter-team standup is simply an organized and structured way of exchanging challenges and possible solutions. So far it works, I’m curious what more this will bring.

Stop Fighting

| Comments

I posted this on Facebook about an hour ago, but this is not limited to Facebook. It should not be. So I’m reposting it here.

Today was a day like any other, except that it wasn’t. It was an official day of mourning in The Netherlands. This morning as I woke up, it was still a day like any other. When I got into my car to drive to work was the first time I noticed a difference: Other music, special requests. As I got to work, nothing was different and actually work was no different. A full day of work, a meeting that lasted until just after 4 o’clock so I completely missed the minute of silence. A day like no other day. Since I went home a bit early on monday and I wanted to finish a couple of tasks that I had left, I stayed a bit longer. That “bit” became quite a bit longer as I ran into a colleague and talked a bit.

I started my way back home in the car. Turned on the radio. Again, the radio felt “odd” yet also comforting because of the special programme. Aside from some tweets and pictures, I had not seen anything of the return of the first victims, I’d not followed it yet. While on the highway, I heard the report of the motorcade of hearses travelling from the airport to the army base where they will try to identify the bodies. Bit by bit, the music, the reports, the tweets, it kicked in.

As I neared the exit of the highway that I need to take to go home, I heard where the motorcade was. A quick calculation later I realized that if I’d stay on the highway, I could reach the route they’d be taking easily. I wasn’t sure. Should I? Isn’t that just sensationalism. I decided it wasn’t. I just wanted to pay my respect, especially since I wasn’t able to join the minute of silence.

I am so glad I did decide to go on. It was a surreal experience. First of all, as I turned onto the highway the motorcade was going to use a bit later, there were thousands and thousands of people standing on the side of the road. Even in the opposite direction, cars were stopping on the side of the road. So many people. Police was trying to ensure safety of people, but they weren’t enforcing the law. Obviously it’s usually not allowed to stop by the side of the road like this. Much respect to the police for the way they handled everything, giving only focus to safety and nothing else.

The motorcade came. That was odd. Strange. Impressive. Fourty hearses, one after the other, several of the drivers with tears in their eyes. I could do nothing but stand there and watch. Silent. Tears in my eyes.

I usually have very little faith in humanity. We’re destroying our world, we’re destroying eachother. What happened is proof of that. But what I felt today, while standing there, the enormous amount of people all there to pay respect, to welcome the dead bodies home, the love, respect and sadness mixed together, shared with everyone there. What a crazy experience. It restored a bit of my faith in humanity. It is possible to have this, unfortunately it will never last long.

I had tears in my eyes for the whole trip home. And as I write this, the tears are again forming. Never, ever in my life will I forget this. But never ever in my life do I ever want to see this many hearses drive by. Ever. Can we please, please all stop fighting eachother?

What We Can Learn From Code Spaces

| Comments

A sad thing happened earlier this week to the (d)vcs hosting service Code Spaces. An unauthorized person gained access to their Amazon controlpanel, and after an attempt to extort money out of Code Spaces to stop a DDoS on their services, (s)he started deleting their Amazon instances. Everything. At the point where the Code Spaces people were able to stop this from happening, nearly everything was gone already.

This hacker rendered Code Spaces dead. According to the statement published on the Code Spaces website:

Code Spaces will not be able to operate beyond this point, the cost of resolving this issue to date and the expected cost of refunding customers who have been left without the service they paid for will put Code Spaces in a irreversible position both financially and in terms of on going credibility.

This is extremely sad for all those working for Code Spaces. Obviously, it’s not good for the Code Spaces customers, but they’ll replace the Code Spaces services with something else. The people involved with Code Spaces itself just lost their job, their company, in the period of about 12 hours. That is horrible.

What can we learn?

At this point, I can only speculate about how this hacker gained access to the Code Spaces Amazon account. But however (s)he did, we can learn from this. The hacker did not start deleting instances until after it turned out Code Spaces was trying to regain access to the control panel. There were backup accounts created already, so even though the main account had been restored, the backup accounts were still there. When trying to recover from such a hack, always check the accounts section to make sure there are no backup accounts present.

The most important lesson…

The most important lesson is actually not one of security. It is one of redundancy. And it’s not just a lesson we learn for our own infrastructure, it’s actually one we need to learn for everything. For all of our software projects.

This time, it was Code Spaces, a relatively small player in the (d)VCS hosting market. Next time, it may be Github or Bitbucket. Sure, I am convinced something like this could not as easily happen to these, but let’s just say this would’ve happened to Bitbucket. How would you have deployed a new version of your software? How would you have run your continuous integration? Every composer install or update would fail horribly, because we rely on the Github infrastructure for that. A huge part of the (PHP) software development would simply stall. I’ve seen it happen before when Github was down. And I’ve seen Github go down just as we were deploying an application. That is a horrible situation to be in. You surely don’t want that.

We need more redundancy. If you run an open source project, it might be wise to publish it to more than one channel (perhaps use Bitbucket next to Github). Again, I don’t expect something to happen to Github, but you may never know. Shit happens, and we better be prepared.

Perhaps Composer (or Packagist) needs to be changed for this as well. Although, perhaps not. Just yesterday Jordi blogged about Toran Proxy which, amongst other things, protects against GitHub outages and such.

Most importantly, we need to be aware. We take things for granted way too much. Things may change, services may disappear. We need to be prepared for this.