Are pull requests the place for training juniors

Question

We have a concept that all code in a Pull Request into master should be production ready. This makes sense and is a fair statement in my opinion.

The idea here being that once you create the PR, you are stating that you would have put this into master, but would like some reviewers to simply 'concur' with you and spot anything you happen to miss.

As we are only human, we make mistakes and hope that other reviewers might find items that unit tests could not find - spelling mistakes, incorrect javadocs etc.

BUT, is a Pull Request the place where we should be providing some level of assistance/training to developers and if so, to what level?

Each time you push new changes, reviewers have to re review your changes, which takes from their development time and causes re reviewing changes.

So, how much training is expected, should be allowed, in Pull Requests? Part of me feels it varies from juniors to seniors. However I also feel that it should not be the place for finding vast amounts of issues - even for juniors.

Is anyone else struggling with trying to get developers to achieve the goal of "My pull request ought to be production ready"?

score 14 · Accepted Answer · answered Jun 11 '17 at 23:46

14

No. Pull requests are not the place for training. Your team has the right idea. A PR means "I think it's good to go. Can I get a 2nd set of eyes on this in case I missed something. I'm human after all." And I suspect that's exactly what your apprentice is doing. They honestly think it's good to go.

Here's an extreme idea (pun intended). Pair Program with the apprentice who's giving you the heartburn. As a more senior team member, it's your job to lift them up and get them to productive.

answered Jun 11 '17 at 23:46

RubberDuck

8,911
5
35
44

Thanks @RubberDuck. Pair program is a great idea and we are doing it - sorta. I suspect we should be doing this more. However some proper metrics or tools to measure if one of your "drops" in the ocean is repeatedly making the same mistakes would help in knowing who needs this pair programming also. Im certain this problem gets worse with larger teams!? – Riaan Schutte Jun 12 '17 at 00:04
2

Well, I would argue that we all need to pair *most* of the time. One of our apprentices has caught more than a few of my mistakes and I'm one of the more senior on the team. You could probably query for number of comments on pull requests, but I wouldn't recommend it. Something about Individuals & Interactions... Seriously though. It's your job to lift this dev up. Get them a copy of Clean Code or something. – RubberDuck Jun 12 '17 at 00:14
1

In a workplace where every dev is working on quoted work for a customer (no internal projects that fund themselves) pair programming is not that easy, but remains important! Seems like something that the company may just have to fork out and invest in if we want more productive developers on quoted work. – Riaan Schutte Jun 12 '17 at 00:24
1

Hmm... why is it not that easy? Doesn't the client get just as many man hours and, more importantly, a better value for their dollar? Best of luck mate. Take it easy on the kid. – RubberDuck Jun 12 '17 at 01:17
@RubberDuck They get the same number of hours, but less functionality. Something that'd take one dev 16 hours costs 32 hours. – Andy Jun 12 '17 at 01:19
3

That's a common misconception @Andy. It's just not true though. Yes, it's counter intuitive, I know. – RubberDuck Jun 12 '17 at 02:20
@RubberDuck Its not a misconception, its my experience. It invariably takes longer because there's more discussion, more distraction, etc. I've never gotten done anything faster when working with someone else. Its one thing to come up with a design with others, but its quite another to actually implement it together. The first is valuable, the latter is a complete waste of everyone's time. – Andy Jun 12 '17 at 21:52
Funny @Andy, I've had the complete opposite experience. I get stuck far less often, because there's a 2nd mind there to stop it from happening. There's far less rework, because there's a 2nd mind there catching things I've missed. And to top it off, there's no wait time for a code review and the related back and forth, wasting hours or days, because there've already been two sets of eyes on it. I'm sorry that you don't like it, but it's *highly* effective for many. – RubberDuck Jun 12 '17 at 21:57
@RubberDuck That's fine, that's your experience, but my experience has been consistent with every company that's tried it. Ditching it and doing code reviews has been more efficient. Saying that it's highly effective for many is just using weasel words to make it sound like your POV is fact. – Andy Jun 12 '17 at 22:02
Don't take my anecdotes as fact @Andy. Perhaps some scientific research would be better. ["We conduct a meta-analysis of 15 years of empirical tests of these relationships, and find that: pairs generally (a) produce higher quality software, (b) learn more, and (c) program faster"](http://ieeexplore.ieee.org/document/7427855/) – RubberDuck Jun 12 '17 at 22:05
@RubberDuck *sigh* Those kinds of studies usually aren't worth the paper they're printed on. The issue is (and has been, for sometime), that there's no accepted, defined way to quantify quality and "faster." Is quality something like # of bugs per KLOC (oh yeah, does your KLOC count blank lines, lines with just a brace, etc etc)? Are all bugs created equal ("this is a few pixels off" vs . "oh shit the query deleted a lot more data than it should have")? How about a bug that only shows in March when Jupiter is transiting Venus, vs one that's every five minutes? – Andy Jun 12 '17 at 22:12
@RubberDuck Did the studies only compare two teams building software to the EXACT same specs to arrive at which one was "faster." Does it still count if faster was also less quality? And finally, how do you quantify how much someone learned? Is there any consideration for how long it gets remembered? Level of mastery? – Andy Jun 12 '17 at 22:14

JohnMcG · Answer 2 · 2017-06-12T17:34:42.637

If code that violates core design principles or standards of the team makes it to the pull request stage, then it should definitely be addressed there. And code reviews can be a good means of communicating the team's standards and design practices.

But is it the best place? Here's some reasons why I would say not:

If it takes several code review iterations to address a fundamental design flaw or large-scale issue, then there will be a strong temptation to overlook the more minor issues mentioned above, due to deadlines or general exhaustion with the review. Ideally, the team would be resilient enough to resist this temptation, but it's probably best not to create a situation that would lead to it.
Receiving a code review with a large number of comments isn't a great experience for a junior / new hire developer. Yes, responding to feedback is a skill they will need to develop, but it's also true that developers aren't always skilled at tactfully responding to code they don't like.

Pair programming and design reviews are preferable venues for larger scale feedback.

That said, it would be even worse to let the code go through for fear that addressing it during code reviews is "wrong," and there may be some circumstances (e.g. remote teams) where this is the best you can do. In that case, address the problems in the code review, and also address the problems in your development process that it got this far.

While finding the problem during a pull request might not be ideal, it is certainly better than in testing or in a production problem.

score 5 · Answer 3 · answered Jun 12 '17 at 02:23

I would phrase it more as pull requests shouldn't be the only place you train people. Also, junior developers aren't the only ones who may need some training in a pull request. Contractors, open source contributors, devs from other departments who are unfamiliar with local code or standards, and even veteran programmers need occasional reminders when they get complacent.

There is very little cost to holding a pull request open for a while. Give it as much or as little feedback as you like, by as many people as you like, then leave it alone until the authors notify you again that they think it is ready for merging. A pull request is a great central tool for communicating about proposed code changes, whether they are fully-ready or need a lot of work.

Some pull requests reviews turn out as little more than a rubber stamp, and some that are external submissions to a team might take a month of back and forth. The first kind is nice when you can get it, but that doesn't mean the second kind isn't valuable. Trying to get people to submit perfect pull requests all the time is just going to be frustrating for everyone.

Thanks for your answer @Karl-Bielefeldt. Agreed that some training can be expected but the question is on how much is appropriate, to what level. Your statement "... whether they are fully-ready or need a lot of work." I would say that a PR to master should never need a lot of work. A lot of work implies design flaws, implementation flaws rather than help me spot what I missed. However I do agree and have experienced "Trying to get people to submit perfect pull requests all the time is just going to be frustrating for everyone." — Riaan Schutte, Jun 12 '17 at 21:39

score 2 · Answer 4 · answered Jun 12 '17 at 21:12

I've always felt that one of the hallmarks of a good lead is someone who provides the additional training as needed during each development cycle. For me, that means I'm not only coding myself, or reviewing code, but sitting with more junior developers, pair programming with them to help them avoid the kind of land mines I've stepped on.

Mainly, I have no illusions that our primary goal is educational - it's not. Whether you're a senior, a lead, or a junior developer, the goal isn't your edification. The goal is always to deliver quality code to the customer. Preferably on time, on budget, doing what they want. I do recognize, however, that it's impossible for me to get all the work done myself, so it's incumbent on me as a lead to help everyone help the team succeed. And that means recognizing training opportunities when they appear in nature.

So, to your question about whether pull requests are the place for training juniors, I would have to say that it's not uncommon for teachable moments to arise during these. Hey, you're going to have to deal with your first merge conflict, let's go over that after the review. Oh look, you didn't include any unit tests for your DAO, we'll postpone your review until after we've had a chance to cover those new methods. Why did you think it would be better to use double primitives in this financial calculation than BigDecimals? Yeah, that's not really uncommon.

So, while I would say that it certainly can happen, but it's clearly not the main goal of a pull request. Nor do I feel there's an expectation that the code in a pull request is production ready. Often it isn't.

If you're using feature and release branches in a gitflow style branching strategy, then your pull requests become something more like release candidates. Not production ready, but something more approximating it. You know your code compiles (right) and you have sufficient test covfefe to make a decent claim that it meets the goals of the user story. And since you've already run several integration tests in your development environment, you have a great demo ready to go should you be asked to demonstrate your changes, which you will, during the review of your PR.

Ultimately, I feel that we should be providing assistance during reviews of the PR, but that assistance isn't around general coding. Instead, it's associated with preparing that proposed code for inclusion with a working base of production-quality code. The PR is an opportunity for developers to demonstrate they have a justification for, and a solid grasp of, each change they've included in the PR. And even then - even after we've weighed them down with unit tests, and demos, and loads of questions - there's still no expectation that the proposed changes are ready for production.

The code's close after all that. But then we let QA torture it.

Thanks for your answer @Michael-Peacock. What you say rings true for companies that have a separate QA department or testers that take it through its next phase. But when the developers are the testers also, the PR accompanies everything from development to testing to merging into master. In this workflow it the code has to be production ready once the PR is approved. I suppose the question is also loaded with an assumption on what workflow you are using. — Riaan Schutte, Jun 12 '17 at 21:35
I would argue that even if you don't have a dedicated QA team, then it's even more important to make sure you add integration and/or acceptance testing to your workflow, and allow time for potential rework should merged code fail your tests. You could automate some of the acceptance testing using Selenium and Cucumber to take some of the load off the developers, but I think it's important to not assume that the code is production ready until you've demonstrated that through testing. — Michael Peacock, Jun 13 '17 at 13:59
I completely agree, hence why we all code requires the associated tests with it. If you then rebase of master prior to merging you can be sure enough that the tests do pass and code ought to work as expected. — Riaan Schutte, Jun 13 '17 at 20:13

score 2 · Answer 5 · answered Jun 13 '17 at 21:17

I want to thank everyone for their contribution and helping me understand what people have to say about this topic.

This is my answer after the feedback given and my understanding now based on the answers and comments received. Thanks everyone.

Summary

Not to expect or enforce perfect pull requests of the bat as this will only become frustrating for all involved.
But continue to make it our goal for perfect Pull Requests.
Expect some amount of collaboration/assistance in pull requests. After all, that's what you are essentially requesting by creating a Pull Request.
If it gets a bit much due to design flaws or implementation flaws, spend time with that developer and do some Pair Programming to build them up and get their code closer to the goal. This is the role of all senior developers.
Juniors will need more Pair Programming sessions than intermediate developers. So expect pull requests to reflect that requirement.
Encourage developers to have design/implementation meetings prior to touching code to reduce any rework identified in Pull Requests.

Wonderful summary & answer. I would not be offended at all if you gave yourself the check mark. — RubberDuck, Jun 17 '17 at 11:44
Thanks @RubberDuck, but my summary could not exist without everyone's answers and comments to my question. Cheers. — Riaan Schutte, Jun 18 '17 at 07:16

score 0 · Answer 6 · answered Jun 11 '17 at 21:23

Can you say more about your company culture in terms of the technical teams? If you're struggling with the idea of code being ready for production when a developer wants to merge into the mainline, then what are you really telling your developers? You're telling them that when their work is "done", it's okay if it doesn't work? It's okay if it breaks the system? If they are adding technical debt, maybe that's ok if they can justify it and are aware of what they are doing, and show that they can come back and do the refactoring later. But if they are oblivious to why they are doing something dangerous, how many times are you going to let it pass?

If you have a junior developer, their first few pull requests will obviously have issues. Eventually they should get the hang of it. If you're finding that they continue to have issues, then you may be assigning them work that they are not ready for yet?

Or maybe you need to hire a replacement junior and lay off the one that hasn't been able to figure it out?

You know what I've seen? People who are not competent developers continue to work at a company for years just because of nepotism. So of course their pull requests require multiple reviews. In Lean parlance, they are "waste"-- a drag on the team, and a drag on the bottom line.

So you have to decide for yourself: how many pull requests will it take for your juniors to show competency, and do you have the courage to let go the ones who don't?

Thanks for you answer @RibaldEddie, We expect that the developers would have written unit tests, manually tested and reviewed their own work to the point where they would assert that this code is great and was it left to them they would merge it into master, but will get 2 reviewers to review it and hopefully confirm that statement. We do not accept any technical debt and are moving fully away from fixes in favor of solutions. So the expectation is that the code meets those requirements. — Riaan Schutte, Jun 11 '17 at 22:32
Anyone from technical leads to juniors. Normally it is one senior/intermediate reviewer along with a junior/intermediate reviewer. (2 reviewers) — Riaan Schutte, Jun 11 '17 at 23:38
@Riaan then over time I would expect the junior members of the team to differentiate themselves. You'll be able to tell who is conscientious and who is lackadaisical. I find software development done well is an activity well-suited to a detail-oriented mentality. Some people may not be able to pull that off. You'll need to decide what to do with them. But fundamentally you should expect developers who submit code to be merged are confident that it works as intended and is production-ready. Even if you have a large, sophisticated QA team, your devs should still be PR'ing production-ready code. — RibaldEddie, Jun 12 '17 at 00:31

Are pull requests the place for training juniors

6 Answers6