How do you measure the value of a PR Review?

How do you measure the value of a PR Review?

Thinking out aloud. You employ a developer whose sole job is to review PRs submitted by other developers. How do you measure the value that developer is providing?

The formula

The time saved by not having to (1) debug issues in production plus (2) the time taken to fix the code plus (3) the time taken to review and deploy the fixes multiplied by the cost of that time.

Plus...

The revenue generated by the customers who would have signed up if that bug did not exist.

Plus...

The revenue that would have been maintained if those customers had not canceled their subscriptions because of the bug that got through.

Plus...?

Minus...

The cost of the review-developer doing the PR.

Minus...

The other noise created by the review-developer trying to make the value look obvious by over-commenting on the PR. (A quick aside: If you are being measured or incentivized to do PRs then you will have a tendency to comment more frequently to show value. Sometimes that will be value but sometimes it will be costly noise.)

Minus...?

Review Feedback and Measurement

What if the PR submitter and added value markers to each comment that the reviewer made?

Given that the submitter has to read through all the comments anyway it should be very little effort to tag the comments with a marker. In Stash there's the "like" link and in GitHub you have a collection of emojis.

An automated scripts could aggregate the tags assigned to the reviewer.

What is a valuable comment on a PR?

This brings up the question on value in comments.

"I like that you used reduce() here instead of filter() and map()" - is that a valuable comment? I think that it is. Perhaps sometimes it needs more context. The value here is that your telling the PR submitter that you (1) read through it and understood it and (2) thought that a particular section was well written.

A comment like this has even more value if the submitter has not used an optimal pattern (that you like) in the past or in another section of the PR. I have seen this where a submitter may have used a "perfect" pattern in one file and then something "messy" to solve the same type of problem in another module. Highlighting the good code as well as the "needs improvement" code has as much value.

The Architectural Decision is Wrong

Every architectural decision we make today will be wrong at some point in the future.

It will be wrong for one of two reasons:

1. Information we didn't know at the time we made the decision.
2. Technology that's changed or become available since then.

This also goes for the technology stack that you choose.

When you make your decision, list all of the reasons why you're taking that path. Do it in your Jira or GitHub Issues (or Whatever tracking system) in the first post or description of the ticket.

Copy/paste the top of this blog post that includes the two numbered points into the bottom of the description and stop wasting anymore time on this. This is a reminder that it's going to be wrong one day.

Months later when you can't remember why you made this decision you can revisit this ticket and make notes about what you could have done at that time to make a different decision if needed. Or perhaps that was the best decision you could have made at that time.

Coaching and the PR Code Review

There is rarely a time when all the members of a team have a deep understanding of all the technologies being used on a project. On top of that the technologies that they're using are evolving and all team members need to keep up with the changes. Some members will learn new techniques and features earlier than the others.

An effective way of cross training team members is through the Pull Request (PR) Code Review.

Comments on a typical PR will be:

• This looks like a mistake because...
• This doesn't conform to our standard because... (This should rarely happen because a lint step should have caught this earlier. If it is happening then the lint rules should be reviewed.)
• This works. I would do it this other way because...
• Please add/modify a test for this.

In addition to that, as a reviewer, you should be asking:

• I'm new to this technology, what does that do?
• Please can you add a comment above this line explaining what this does?
• I haven't seen this syntax before, is it the same as?
• This is fantastic! I never knew you could... (Call it out when someone does something you haven't seen before or does something really well and you learned from it. Everyone loves positive feedback.)

If you submit a PR that has new features or techniques then consider immediately adding comments (in the PR) about them to help the reviewers if know that they will not understand what something does.

The question then arises, should I add a comment in the code or a comment in the PR? My take on this is that comments about how a language, framework or technology works, in isolation, should go in the PR comments. You don't want to clutter the code with information that can be found through a search on the web. The comments that go in the code are how the code works.

These are the advantages of learning through code reviews:

1. The reviewer is learning in the context of the project domain. You don't get more real world than that. Not only will the reviewer be learning new syntax/constructs but they will also be understanding the business domain and gain knowledge in part of the code base that they may need to maintain in the future. Compare this to learning contrived examples in a class.
2. The reviewer is learning a subset of the technology as it pertains to this domain. We would all love to learn all aspects of each technology we touch. Time constraints do not make this possible. Learning like this in situ provides the most time efficient way of learning the essentials. Again compare this to class learning where you may gain some super interesting knowledge and then not apply it.
3. The coach, the person who submitted the PR, is forced to look at and explain their code. When you ask your seemingly naive question you may see a comment like "This construct in this language does... Now that I look at it again I see a problem/way-that-I-could-improve-it."

Using this technique you get a better code review on the PR and a great contextual training session at the same time.

MomentJS Notes

Install

npm i moment --save
npm i moment-timezone --save

Use

var moment = require('moment-timezone');

Create moments in and out of DST:

var cdt = moment.tz("2015-07-23 08:30", "America/Chicago");
var cst = moment.tz("2015-11-23 08:30", "America/Chicago");

Check that offset from UTC is what you'd expect:

cdt.tz('America/New_York').format()
> '2015-07-23T09:30:00-04:00'
cst.tz('America/New_York').format()
> '2015-11-23T09:30:00-05:00'

Check that zone aware format output is what you'd expect:

cdt.tz('America/New_York').format('HH:mm:ss')
> '09:30:00'
cst.tz('America/New_York').format('HH:mm:ss')
> '09:30:00'

Node Foundation Membership Election

Mikeal Rogers recently posted Nominations for the 2016 Election for individual representation on the Node Foundation board.

I'm putting myself forward and answering the question "Why would you like to represent the individual membership of the foundation?"

Why?

I think that Node.js and the ecosystem around it plays a critical role in our current technology stack. This is going to become more important in the future.

I would like to be able to help shape the success of this platform. One way to do that is to understand what the membership wants and needs to get out of what we have today. More importantly what do they expect from the future. As part of the bridge between membership and the board I will be working to ensure that members' opinions are represented at the leadership level.

Qualifications

Leadership: In the past I've served as a director of engineering for a large IT company managing a team of 35+ managers and developers.

Technical: I have a few open source projects that I manage. I also contribute to open source projects I can improve through pull-requests. My day-to-day work is mostly writing JavaScript against the Node.js platform and system design and architecture.

Questions

I've intentionally kept this brief. I'll answer any questions. If you ask them as comments to this post then we can keep them in one place. Anyone else reading this will be informed.

Babel Cannot read property error of undefined

This is more a reminder to myself about how to solve this error that I've stumbled across a couple of times with Babel 5 & 6.

If you downgrade Babel from 6 to 5 and leave a .babelrc file in your project with a Babel 6 specific setting like:

{
  "presets": ["es2015"]
}

then you might get an error like this:

if (!option) this.log.error("Unknown option: " + alias + "." + key, ReferenceError);

TypeError: Cannot read property 'error' of undefined

The solution is to remove the .babelrc file or remove the Babel 6 specific settings.

Also, if you're using Babel 5 and you require a module from another project that has a .babelrc file which is using Babel 6 and has a Babel 6 setting in it then this will cause the same problem in your Babel 5 project.

Docker and GLIBC_2.14 not found

Hit a problem today when I was using Docker and Docker-compose to run a Node.js app in a CentOS container. I had done an "npm install" locally and then the docker-compose command had setup the container and copied everything over from my Ubuntu workstation.

Problem was that I was using a couple of Node.js modules that used native code. So the Ubuntu compile of those modules was getting copied to the CentOS container and when CentOS was trying to run them it was giving me an error about "GLIBC_2.14 not found" because it hadn't compiled those modules.

I feel that a good practise for almost any Docker setup is to have a .dockerignore file which excludes the "node_modules" folder.

This is what my .dockerignore file looks like:

node_modules/