justphilll

[justphilll]

How I review code

Reviewing code is one of the most important parts of an engineer’s job at Tumblr, even more so than writing code. Our codebases are shared by hundreds of engineers, so it’s critical to make sure we’re not just writing the best code we can, but that the code being written can be understood by others. Taking the time to review someone else’s code is the most critical opportunity to ensure all of that is happening.

At Tumblr, every code change happens in a Pull Request on an internal Github instance. We have repositories for the PHP backend, our database schemas, our iOS (Swift/Obj-C) and Android (Java/Kotlin) mobile apps, infrastructure projects written in Go, C/C++, Lua, Ruby, Perl, and many other projects written in Scala, Node.js, Python, and more. All of our code repositories rely on authors to write Pull Requests and get approvals from their peers before merging their changes to the master branch and deploying to production where real people interact with it.

How I personally review code has changed considerably over my few years at Tumblr. Before working at Tumblr, I wrote code mostly by myself and reviewed code with a very small set of people. Shifting to a huge codebase with hundreds of contributors was a big change. Thankfully I’ve had some good teachers. I went from reviewing maybe one pull request a month to currently reviewing an average of 25 pull requests a week. Here are some of the principles that help me keep my reviews timely and helpful.

Review the code with its author in mind

The first thing I ask myself after a review has been requested of me is who wrote this? Are they a junior or senior engineer? Are they new to this codebase or a seasoned veteran? Have I ever reviewed their code before? Am I familiar with the project this code change contributes to?

When I’m reviewing the code of someone I work with closely, I probably know pretty well what their thinking was when they wrote it, and I have an idea of what experiences they’ve been through. Junior engineers sometimes need a little more hand-holding, which usually means giving them more help with code examples and references. Senior engineers sometimes need to be reminded that highly performant, abstract, or clever code is often difficult to read and understand later, which usually means asking them for more inline comments and documentation.

It’s also fundamentally important to review the code as if anyone could read the review you’re about to submit, not just the author. There are two main reasons for this. First, some people learn by reading the reviews that other engineers write; as a more junior engineer that’s exactly how I found out the most about the intricacies of Tumblr’s codebase. Also, in six months’ time it’s very likely you may be looking at this code again to figure out how it works. Having a helpful code review of it around can give some insight into the decisions that went into why it works the way it does.

Review the code with everyone else in mind, too

The core of my review, no matter who is writing the code change, centers around being able to understand the code itself and the motivations and context around it. To me, ideally anyone should be able to pop into a pull request and expect enough context to understand the code change and why it was done the way it was done and how it works the way it works. This is especially important in an old, shared codebase, where someone three years from now may be looking at your PR to figure out why you chose to do what you did. If that’s not included, or if there aren’t at least links out to the relevant context, something is wrong. More detail is always better.

I don’t worry as much about code style or syntax itself, as we have automated processes to ensure that new or changed code conforms to our agreed-upon coding standards. Similarly to what I wrote about in how I code now, I look for code that is well-documented (both inline and externally), and code that is clear rather than clever. I’d rather read ten lines of verbose-but-understandable code than someone’s ninja-tastic one-liner that involves four nested ternaries. Especially if the person writing the code has been around the block a few times and been burned themselves by old, undocumented, clever code.

Once I feel like I can understand the code change, I try to put myself in the shoes of someone who doesn’t deal with this area of the codebase very often (which may be the case for me at the time!) and think of how to review the code to help make it clear for them. I try to think of someone new being hired six months from now, looking at this code, wondering how it works.

Understand the PR’s scope

Sometimes not everything can get done in one pull request. At Tumblr we try to keep our PRs small so they can be reviewed quickly and safely, rather than bundling a ton of hard-to-review work into a 5,000-line-change PR. Because of this, sometimes the work has to be broken up into chunks, with PRs that build a foundation and lead to future PRs with finished implementations.

Or, alternatively, it’s common for evergreen codepaths to have known issues or work that’s been ticketed for future sprints, so it’s become a good, common practice to leave a @todo in the code with the name of the ticket where that todo will get done. That way we can unblock code changes from having to be totally complete within one pull request.

Stay on top of the whole review process

The number one thing that helps me review code in a timely manner, and stay on top of updates about PRs, is email. I check every Github email I get; I make sure that I don’t get notified for everything that happens in the repo, but I do get every email that happens relating to a PR I’m associated with. This helps me stay on top of every step in the review process, because it’s almost always a back-and-forth that ideally shouldn’t last more than a day.

At Tumblr, most of our reviewers are selected by automated round-robin assignment when the PR author is ready to receive reviews. That assignment triggers an email and subscribes me to everything that happens relating to that PR. From there, it’s on me to stay on top of my email and make sure that I not only allocate time to do the review as soon as possible, but follow up on the PR if I leave a review and the author updates it in response to my review.

Remember to be a human

The most important advice for reviewing code (and, in other ways, writing code) is to remember to be a human. Remember that the person who wrote the code you’re reviewing is also a human. Give them the benefit of the doubt. Be nice when you write a suggestion, or have a question, or find an edge case that they don’t seem to have covered. Even if they’re a seasoned veteran coder who has written bulletproof performant code for years, treat them like a person who makes mistakes sometimes. Even if they’re someone you work with every day and you feel comfortable cracking jokes at their expense, understand that a new person might not understand.

Remember that shared, living codebases are often hectic and strange, especially ones that have been around for a decade. Remember that sometimes things are in a rush, so you can only do the best you can. We can’t halt everything in the name of perfect code, but we should make sure that everyone is doing the best they can, whether we’re writing or reviewing code.

[justphilll]

Tip: Always specify the type attribute for the Button element. Different browsers may use different default types for the Button element.

This has caught me out more than once: https://www.w3schools.com/tags/att_button_type.asp

[justphilll]

This is useful for creating repeating tasks in Trello

[justphilll]

If you're on Windows 10 and your IIS does not seem to support multiple connections and the ability to connect remotely to a web site then you probably need to install this feature.

[justphilll]

It is seemingly of limited use as there don't appear to be any new deveelopment features that Chrome does not already safely support.

[justphilll]

Ever wondered why when you have compression available on a website that when you view it does not appear compressed? It probably because you have "Decode" turned on so Fiddler automatically uncompresses the response. If you turn it off then www.google.co.uk shows as GZIP.

[justphilll]

This is one of many processes. Roughly, this is how we review our code libraries.

Here, part of what checking that code looks safe is the presence of strong unit tests. Unit tests strengthen the codebase. It may take three times longer to use pure TDD. We suggest that there is a middle ground. We are not even suggesting that ever core business requirements has a set of unit test. Instead, we say that any method that saves to the database needs at least two unit tests, one for pass and one for fail. We do not advocate null tests as such because good client side scripting should not allow null conditions to be passed to the web server. Of course, this is all fair and well when we are talking about websites that live as part of an intranet where users are not expected to be able to turn off JavaScript. There should be null validation on the server but we don't want to explicitly test for it in unit tests.

[justphilll]

"The changes you have made require the following tables to be dropped and re-created" When changing tables in SQL Server Management Studio 2014 you may get the following error: Saving changes is not permitted. The changes you have made require the following tables to be dropped and re-created. I was surprised when I saw this message first but there is very simple solution. From top menu select Tools and then Options. Select Designer and Table and Database Designers. Uncheck the box Prevent saving changes that require table re-creation. Now you can edit your tables without being stopped by re-creation limits.

[justphilll]

Sometimes you need to query (using a SQL style language) your IIS logs to understand what is going on. In short, We mean that i tis not practical to import, say, 4GB of data into your database just to run queries. This is where Log Parse comes in handy. Here is an example: run > cmd Microsoft Windows [Version 6.1.7601] Copyright (c) 2009 Microsoft Corporation. All rights reserved. C:\>cd\program files (x86)\log parser 2.2 C:\Program Files (x86)\Log Parser 2.2> C:\Program Files (x86)\Log Parser 2.2>LogParser.exe -i:W3C "SELECT TOP 25 EXTRACT_EXTENSION(cs-uri-stem) As Extension, COUNT(*) As Hits FROM c:\inetpub\logs\Log Files\W3SVC1\* GROUP BY Extension ORDER BY Hits DESC" -o:CSV Extension,Hits ,6 ico,3 png,1 Statistics: ----------- Elements processed: 10 Elements output: 3 Execution time: 0.01 seconds C:\Program Files (x86)\Log Parser 2.2>cls LogParser.exe -i:W3C "select * into C:\Storage\travellogs\Logs\OUTPUT_09.CSV FROM C:\Storage\travellogs\Logs\ex120619.log where time > '21:00:00' and time <'22:00:00'" -o:CSV Statistics: ----------- Elements processed: 1908736 Elements output: 131252 Execution time: 17.74 seconds C:\Program Files (x86)\Log Parser 2.2> LogParser.exe -i:W3C "select * into C:\Storage\travellogs\Logs\OUTPUT_09.CSV FROM C:\Storage\travellogs\Logs\ex120619.log where time > '09:00:00' and time <'10:00:00'" -o:CSV C:\Program Files (x86)\Log Parser 2.2>LogParser.exe -i:W3C "select * into C:\Storage\travellogs\Logs\OUTPUT_09.CSV FROM C:\Storage\travellogs\Logs\ex120619.log where time > '09:00:00' and time <'10:00:00'" -o:CSV Statistics: ----------- Elements processed: 1908736 Elements output: 83747 Execution time: 25.82 seconds C:\Program Files (x86)\Log Parser 2.2>

[justphilll]

We may start using this when it supports ASP .Net Web API.

[justphilll]

Starts with a numeric character

SELECT TOP (200)   * FROM dbo.Address WHERE (ISNUMERIC(LEFT(AddressOne, 1)) = 1)

This is how to check that the first character of a column starts with a number. This is useful for addresses.

[justphilll]

Cacoo has moved on considerably side we last used it, several years ago.

[justphilll]

We're convinced that one day, we could make use of this!

[justphilll]

This is proof that you can use UML to model just about anything. Here's a UML class diagram of rhetorical devices that involve figuration and mapping. I had fun with my son, showing him relations between the family of rhetorical devices. The diagram is incomplete but it's not bad for a first draft.

[justphilll]

ngrok is useful for exposing an IIS Web API application on your local machine to the outside world. You application can be called externally (hooked into) from another server elsewhere. For example, Signable allow you to forward to your local machine through a "Webhook". For another example, Dynmark offer what they call "Url Forwarding".

1) On Windows Start Menu, click Run

2) We want to open the Command Prompt. So, type "Cmd" and enter

3) In the Command Prompt, do the following:

cd c:\ngrok_2.0.19_windows_386

4) Then type the following:

ngrok.exe http 80

# Something like the following is then displayed:

ngrok by @inconshreveable

(Ctrl+C to quit)

Tunnel Status online

Version 2.0.19/2.0.19

Web Interface http://127.0.0.1:4040

Forwarding http://8dd33939.ngrok.io ->

localhost:80

Forwarding https://8dd33939.ngrok.io ->

localhost:80

[justphilll]

In the TeamCity build log, if you see "has been running for more than 15 minutes. Terminating" or similar then you need to change Administration > Global Settings > Default build execution timeout. End to end tests can take much longer than expected.

[justphilll]

I'd like a Windows Mobile equivalent for C# and the like.