10

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"?

13

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.

  • 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 0: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 0: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 0: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 1:17
  • 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 2:20
9

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:

  1. 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.
  2. 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.

5

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
2

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
2

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.
  • 1
    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 7:16
0

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
  • @Riaan who are the reviewers? – RibaldEddie Jun 11 '17 at 22:54
  • 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 0:31

protected by gnat Jun 12 '17 at 22:29

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.