76

I've recently joined a new company within this pretty small team. I'm running into this issue where my coworkers approve my pull request a few seconds after I ask them to review it. There never are any comments on it, no matter how complex the pull request is.

I'm getting a little frustrated with this because I know my code isn't perfect, duh, but I'm not being able to grow in my coding abilities if I get no feedback. I feel like I can just write any code and I get the approvals to merge it in.

We are currently without a direct manager so I can't talk to them about this. I also feel weird about going to the developers and asking them to review my pull requests better. Maybe I shouldn't?

Note that pull requests from other teams have tons of comments, good comments, not nit picks.

Not quite sure what to do here.

  • 5
    You can try introducing an obvious ( but harmless ) bug into your code to see if they are actually reviewing the code. – sf02 May 30 at 13:42
  • 40
    I'm positive they aren't reviewing it. I once put a 10 file change with 509+ lines of code change and it took then 20 seconds to approve it. I'm not kidding, 20 seconds. – mkki May 30 at 13:45
  • 34
    I really don't want to introduce any bugs. I'd rather go a different route. – mkki May 30 at 13:46
  • 6
    @Chris I think there's a good deal of overlap. If it was simply asked as "my colleague is meant to check my work before signing off on it, but just signs off on it without checking" and didn't mention software development, it would be fine here. – John_ReinstateMonica May 31 at 7:14
  • 8
    Recommend that the comment "I'm also the most senior dev in the team" be added to this question text. – Daniel R. Collins May 31 at 18:59

14 Answers 14

47

The short answer is: No

It's the job of the person executing the merge to ensure that the process has been followed. You did your job, now they have to do theirs.

You could follow up with them and ask why there are no comments or feedback on your pull requests if you feel there should be. This is a cultural question - how does your office work normally? Do people do actual code review? What do other people's PRs look like?

  • 99
    Here we see a standard Workplace Q&A pattern: someone says "something is going wrong at my company; what should I do?" and the answer that appears is "simply do nothing unless your official responsibilities require you to". Does nobody here actually care about the success of the organisations they work for? Is nobody here doing anything that actually has an impact on the world, such that things going wrong matters at least a little, even if not in a literal "people will die" sense? I will never buy into this idea that one should ignore dysfunction because it's not their job the remedy it. – Mark Amery May 31 at 11:13
  • 13
    @MarkAmery That's a noble attitude, but after a few decades seeing the results of dozens of well-meaning people who think they know how to do everyone else's job better, it's often not a useful attitude. If you really want to put the company to rights, first get yourself promoted to a level where you have the authority to do what you want and impose it on other people. – alephzero May 31 at 13:29
  • 20
    @alephzero Sure - throwing a tantrum over every decision and process that you disagree with and refusing to drop it until you get your way is even worse; some level of willingness to defer to others is necessary for any organisation to run smoothly. But it doesn't follow that the right answer is what this answer advocates - to make no attempt at all to influence things that are going wrong even when they are contained within your own small team, squarely within your own area of expertise, and impacting the outcome of your own work. The right approach lies somewhere between those two extremes. – Mark Amery May 31 at 14:06
  • 22
    @Dan I'm speaking from plenty of experience. Honestly, I find this bewildering. Am I to infer that you've never pointed out a problem to somebody which they then fixed, or made a suggestion that they then implemented, without having first been granted formal authority over them? That every idea you've ever raised in your career has been rebuffed unless the person you expressed it to was officially your subordinate? Suggesting ways of doing things better to peers and managers has been something that people have done at every company I've worked at, and is unremarkable. – Mark Amery May 31 at 14:48
  • 22
    This answer is a clear example of why you should post software engineering questions in the software engineering SE site. Nobody who understands agile software development would tell you that this is not your job or not your responsibility. You and your team are responsible for your process and for improving it. – Blueriver May 31 at 16:51
116

While the other answers are good, you have mentioned something new and relevant in a comment, which might be causing the specific behaviour from your teammates in your case. You said:

I'm also the most senior dev in the team

That could be important here. In that case, I can easily imagine that you are treated with extra deference / respect, and a mixture of the following could apply (I've seen these personally):

  • the other team members don't want you to see them as challenging your code quality; and/or
  • the other team members assume that what you write will be better than what they write and so they (wrongly) assume this means you won't make mistakes which they could find, even if they did review your code;

    and therefore:

  • they just "rubber stamp" your pull requests - which is what you see happening.

This is especially true since you have:

recently joined a new company

So since you're new in the team, they may not know that the senior developer still wants in-depth reviews from less senior team members (not all senior developers do). Perhaps your team have previously had a senior developer who "blew up" when their code was reviewed, and so they have become conditioned not to do it?

I also feel weird about going to the developers and asking them to review my pull requests better.

I suggest taking one team member aside in a quiet private moment, and with no hint that they have done anything wrong, just ask them why they didn't review your code any deeper? And hope that they give an honest answer. Their answer might match one of my suggestions above, or might bring out something new e.g. perhaps you did something in the past (unknowingly) which made them think you didn't want in-depth reviews, and so they have been following that misconception ever since?

I don't think you will know the answer until someone in your team tells you why they behaved that way.

  • 19
    @gbjbaanb - "It might be worth getting everyone together" Personally I deliberately would not start with everyone together. Peer pressure can give odd results e.g. individuals who do not agree with the loudest talkers, may not speak out in front of them. This may also inhibit any explanation of why code reviews (at least those of the OP's code) aren't currently being done in that team, especially if the reasons are embarrassing / personal / etc. That's why I suggested to start with a quiet, private one-to-one, to avoid peer pressure and make it easier for them to be open. As always YMMV. – SamGibson May 30 at 23:59
  • 3
    If OP is the recognised "most senior dev" part of their remit may be to train the team and improve the quality of the work there. Having knowledge of good Code Review practices is something that could be shared. I feel it might be worth emphasizing that as part of your answer which otherwise covers all I could say. – TafT May 31 at 8:01
  • 1
    Nice easy way to test this - put a load of rubbish in a pull request and see what the reaction is. If it gets approved, you have something you can present which you know shouldn't have been. – UKMonkey May 31 at 9:23
  • 3
    @UKMonkey he's already done that effectively. If a PR is approved moments after issued, it means that it doesn't matter what was in the PR, rubbish or not. – iheanyi May 31 at 16:49
  • 2
    I'd suggest telling them that you need constructive feedback on your PRs to help you improve, rather than asking them why they are rubberstamping. The latter can come across as accusatory, while the former emphasizes that this is about self-improvement. – asgallant May 31 at 17:03
9

You're probably going to have to let it go until you get a direct manager.

Keep in mind, the person approving your pull request is trying to "do their job" - and apparently, in their mind, "their job" doesn't involve doing a lengthy review of your code. But that's the problem: you can't force them to change their opinion about what their job consists of.

Realistically, you're going to have to wait until someone in authority has the ability to direct what the priorities are - and is able to indicate to the other programmer that code reviews are a part of what they're supposed to be doing.

  • 4
    Usually this kind of problem goes on until a serious system issue causes management to get involved. Only then do they care because it affects them. – Underverse May 30 at 13:59
9

As the most senior developer on the team, I would say it is part of your responsibility to set expectations for the development workflow on your project.

I have worked on projects that are collaborations between senior and competent developers, where pull requests are pro forma unless a PR specifically requests that others take a close look at the code and double-check something. Detailed comments and reviews (especially on code style etc) were not expected or welcome for every little change. I've also worked on projects where every PR, even one-line bug fixes, are scrutinized.

In your shoes I would set expectations with a polite and humble nudge, e.g.: "I'm about to send you a pull request for [Feature X]. I'm a little inconfident about how I fixed [Issue Y]; could you please take a look and let me know if you spot any problem? Also, do you have any advice about how I might refactor [Section Z]? I think it might be hard to understand and maintain in the future but I don't see an elegant solution."

7

I agree with you this is a problem but I don't think the problem is that there are no comments. I think the problem is that the pull requests are instantly approved. That means no one is critically reviewing your code. And if they aren't reviewing your code, they probably aren't reviewing each others' code either. You aren't learning / improving, and bad code is landing in the main branch.

What you need here is allies. Figure out who on this team does take code review seriously. Put code review on the agenda of your next team meeting and talk it out. If the majority sides with you, more likely you win. If not, then even at most start-ups, there is some management. Get them to understand that this "buddy approval" issue is very real and a threat. Show them examples of instant approval or even of obviously bad code that was not caught.

With majority or management backing, go to whoever administers the repository and ensure that rules are put in place on the code repositories that require at least one trusted reviewer (who takes code review seriously) to approve any pull request before it is merged. All the other "buddy approvals" therefore carry less importance. However, I think it is still important that those who don't take code review seriously still be required to do it, in the hopes they improve.

I have battled this very issue on my team and having trusted reviewers was my solution. You have to protect the repository from habitual "buddy approvers". In my experience, if you can't make that happen, there is probably no way you can win the code quality battle. The best you will be able to do in that case is cover your own backside and wait for the inevitable disaster that forces the issue. Don't be the one holding the bag when it happens. Don't compromise your own code review discipline; maybe some of it will even rub off on them. Don't merge your own pull requests until you have more than the required number of approvals. And make sure you write better and more comprehensive unit tests than the next guy.

If you can do this without even the appearance of casting blame, then the next time a glaring defect that should have been caught in code review makes it into the main branch (ie code with bad anti-patterns), bring it up with the whole team as an opportunity to improve quality with a renewed focus on the code review process. Just make sure there is no blame, or you only hurt your own reputation.

Specifically concerning the need for comments, I think comments should generally only be made if you see a problem or have a question. If the PR is good, there is no reason to comment; just approve it. However, if a PR is declined without any comments, I think that would be a problem because the contributor does not know what should be changed to make the PR acceptable.

(And, this question probably belongs in Software Engineering SE.)

  • If you can't trust your (paid) developers to do good reviews (with training, expectations explained that they do reviews, etc.), just fire them and hire better ones. Otherwise you have Sally and Bob and Tom and Jane who spend all day doing the fun code stuff, and poor diligent Jim and Joan who spend all day just doing code reviews, which take all day because they're being diligent, and then explaining in stand up every morning that they spent the whole day doing code reviews and not making any progress on their actual assigned tasks......nah, I'm not bitter (much). – user3067860 May 31 at 15:58
  • 1
    Yes, this. The goal is to have NO comments. But it's to have no comments because the code is good, rather than because reviewers didn't bother to review it. – Mohair May 31 at 16:39
  • @user3067860 The problem you mentioned can be combated through task accounting. If your scrum tool supports task breakdown in stories with hours, just make sure code review is a separate distinct task in each story, and that code review is part of the definition of Done, and get serious about logging task time accurately. Jim and Joan will then contribute all their capacity into code review tasks on the stories, and the others will be exposed as unserious reviewers. If management asks why Jim and Joan "take too long" in code review, it's the perfect opening for a discussion. – wberry May 31 at 20:46
  • @wberry But I would rather quit and find a better job with better coworkers than continue to be the only one doing all the peer reviews, while my colleagues get to do the actually fun stuff. Also, I would rather quit and find a better job than record "time spent by hours", and if the reason why I have to record time spent by hours is because my coworkers are juveniles who can't do their share of the not-fun work...... – user3067860 Jun 1 at 1:18
6

Do what I do - directly ask a senior developer if they can take a look at your proposed changeset and see if they are happy with it. Ask for feedback. I've yet to have a negative experience with this approach.

If they don't have time to take a look at your code, they'll let you know that.

  • I might have to reach out outside of my team to get this. – mkki May 30 at 13:49
  • @mkki your team mates would be the ones most familiar with the modules and coding standards of the project you're working on. – user1666620 May 30 at 14:00
  • 3
    that's the problem though. My team mates aren't comenting on my PRs. I'm also the most senior dev in the team, so I'm going to have to reach out outside of our team. – mkki May 30 at 14:02
  • 14
    @mkki ah, I see. Well, as the most senior dev you should be setting the tone. Talk to your colleagues and explain your concerns to them and remind them of the purpose of pull requests and code reviews. The issue could very well be that, because you are the most senior developer, the others assume your code is fine or they could be wary of pointing out errors. You say that you are without a direct manager - but somebody must be telling you what to do, right? You're not operating in a vacuum. – user1666620 May 30 at 15:16
4

I suppose I'm a little against the grain here, but yes this is a problem and yes you should attempt to be a part of the solution. Before going on, I'll say that this is a difficult problem to solve. Here's the principle to follow with code reviews:

If we are approving the majority of code reviews with no input, that means we believe we generally do things correctly the first time.

Generally speaking, that's a bad life philosophy. For software, we know that code which has been tested and deemed production worthy always has bugs. I had a professor in college that claimed it was about 1 bug per 100 lines of code in most software, but I can't find a source for that right now. Also, bugs are expensive: some claim that a bug costs $10k if found in production

It's worth it to spend some time reviewing code and at least asking questions about it.

As far as solving the problem, you obviously can't force people to change since you aren't management (and of course, management can't really force people to change). I recommend a few things:

  1. Review code the way you feel code should be reviewed. In the conversations that follow, explain to people why you do it the way you do, and make it clear to them that you'd love for them to review your code the same way.
  2. Discuss this problem with others who are influential among the engineers. At some companies, these people would be architects or other senior devs, but whatever the position is titled you are looking for technical leaders that other engineers on your team respect. Discuss what's happening with this person (these people) and try to get them to help.
  3. When you do get a direct manager, make it a talking point with the manager and convince your manager that this is something that needs to change.

Recognizing these kinds of problems and attempting to solve them is a major differentiator among engineers.

  • I disbelieve your answer because "within seconds" – Joshua May 30 at 22:19
  • @Joshua I'm not sure what you are referring to? – dbeer Jun 4 at 15:16
  • They couldn't have reviewed the pull request in less time than it takes to read it. – Joshua Jun 4 at 15:31
  • I don't see what part of my answer you're referring to. – dbeer Jun 4 at 18:05
4

You need to find out why they're not reviewing pull requests before you can decide what to do about it. Off the top of my head, it could be any combination of:

  • They are deferring to you as the most senior.
  • They have another process to review code.
  • They have really horrible unsafe practices and just never review code.
  • Your pull requests are too complicated to review in the tool.
  • They are under pressure to release as fast as possible, skipping otherwise essential steps.
  • They don't know how to do a real code review.
  • They are too lazy to do a real code review.

I think the best case you could hope for would be if they have another process for reviewing complicated commits, but they haven't explained it to you and are quietly wondering why you aren't following it since you are more senior. In which case if you have a quick conversation they can explain how they do code review to you...and I would recommend giving their way at least an honest try before immediately declaring that your way is better.

I think the more likely, more unfortunate case is not good practices, not knowing better, and trying to skip steps to save time or effort. Then as the most senior it is your job to teach them and encourage good practices (maybe even enforce good practices, if you were hired as a lead). You will probably need to do code reviews as group for a while to set your expectations for how thorough they need to be. And you will need to adjust your velocity, since for a while you will be spending a lot of time on code reviews. (You make it up later by not having to spend time on bug fixes, but with no released product...yikes.)

3

If you're working in an agile environment, fixing this is your responsibility, as well as the rest of the team's. You guys are responsible for your own process, you own your work, you are a self-organizing team of capable software developers and should be improving in every way you can, both individually and as a team.

The core problem here is that they're either not following the process of reviewing every line of code, or the process is not well defined (for example it only states that PRs must be approved, not that the changes must be actually reviewed). Either way, you guys need to decide how you're going to work from now on, define a process you all agree on, possibly write it down (mostly for future hires), commit to following the process you yourselves just decided on, and do it. Bringing this up is the responsibility of every single member of the team, including you. Some (for example juniors) might not notice this opportunity to improve how you work, so bring it up yourself and teach them why this is good for the team as a whole (better code, less bugs), you as an individual (learn to write better code) and them as individuals (get used to reading other people's code, learn how well-written code looks like).

As others have mentioned, this might be related to you being the most senior developer. Some might think that questioning you is bad, and not realize that it's actually a great opportunity for them to understand why you do (most) things better than they do and how you think about problems to arrive at (mostly) better solutions. Teach them this. And also, strive to create a safe environment, where even if they sometimes don't clearly see opportunities like this one, they feel free to question anything and anyone without fear that the other person might feel attacked. You might want to ask around how other teams in your company do that, and how you guys can achieve that.

1

Your work should be properly reviewed before it is accepted. Since your co-worker apparently is unwilling to do it, you have to review your code yourself. It’s a good idea to do this anyway; the better the code you let others review, the better for your reputation and the less work for everyone.

That doesn’t mean you shouldn’t get him to review your code properly.

  • 5
    Obviously a developer needs to review their own code, the process is part of normal development. And I feel like "get him to review your code properly" is unhelpful. – l0b0 May 31 at 4:58
1

A few things from personal experience which can help with your problem:

  • Keep PRs small and simple. Reviewing a small PR that solves a single, well-defined problem is a lot easier than going through many different changes in multiple files. The bigger the PR, the more likely it is that your peers will skim through it and miss important changes.
  • If there are specific changes you want feedback on - comment on those and explain your concerns. This will draw people's attention to the change and prompt them to share their opinion.
  • Tag more people on your PRs and require at least 2 or 3 approvals before merging.
1

My employer will assign two engineering leads to every project. One is the eponymous "Engineering Lead" who's in charge of tasking the project out and answering directly to production and QA, and is essentially a "Buck stops here" guy. The other is a "Principal Engineer" whose job role is essentially "Be the guy who knows everything."

Principal Engineers often set architectural guidelines for the project, dictate certain technologies be used (or not used), and enforce best practices in the code base. To a certain extent, that code base is their baby and they should not allow a blemish on the shell of any of its many many turtles. To that end, the Principal should themselves be reviewing some decent proportion of the PRs coming in, or at the very least be reviewing the reviews.

If you have an engineer in a similar role, I strongly recommend that you go to them with your concerns. They appear to be well-founded. If you do not, I strongly recommend that (given your seniority) you fill that role yourself, with the buy-off of your department head.

1

There are a lot of good answers here, but I didn't notice anyone that suggested that in your PR:

  • Mention what you fixed
  • Mention how you tested it
  • Ask for them to look at/for specific things

Another thing that you could do, and I do this with my own PRs, is after submitting your PR review it yourself.

That means taking off your, "I wrote this," hat and putting on your, "Someone else wrote this," hat.

Pretend that it was your most junior dev on the team who wrote the code, and review the code exactly like you'd expect it to be reviewed. Comment on your PR the way you'd expect to have people comment on it.

There have been a number of times when reviewing my own code that I've discovered things that I forgot to fix/debugging code I forgot to remove.

It also sounds like you do have a cultural shift you need to make. As the senior dev you have some weight to throw around, but you need to decide if you want to expend the political capital to require things by fiat, or if you want to spend the time to build the team skills that you're looking for. The latter is probably more effective, though it may take more time.

1

As this is Workplace.SE not Software Engineering SE, I will answer this question from a general "new engineering hire" point of view.

You're in a new job on a small team. As such you need guidance on the way things are done in this company and in the project you're working on. But as a small team who has probably had to deal with being short handed until you were hired, it's likely they have a backlog of work that needs doing. Have you asked this?

We have a new hire in our company (engineering, not software.) At first he was only assigned one project - one of my projects - and was clearly frustrated that he was not getting the supervision he would like. But I had urgent things to do (since we have been short handed for several months) and could only help him with things that need to be done perfectly right NOW (i.e. stuff that was going outside the company to the client and suppliers). He proactively took it upon himself to prepare bill of materials for internal company use for the project (not on the company form as he wasn't shown where it was.) His format turned out to be a vast improvement on the company form, so I told him to stick with it but make some adjustments to the content. He now has several projects to work on so the issue is resolved - in fact just a couple of weeks later, he's too busy to accept my guidance when I offer it and we have to set a mutually convenient time.

Point is, you need to find out if what's happening is a temporary situation or if it's what they always do there. They clearly feel that writing "their code" is their responsibility and more important than reviewing "your code" which is clearly wrong. Document what is happening so that when someone complains that your code doesn't work or meet company standards, you can counter with "I was new in the company and nobody gave me any guidance." That's the best you can do.

protected by mcknz May 31 at 20:42

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.