77

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.

Stelios Adamantidis
  • 194
  • 7
mkki
  • 619
  • 1
  • 5
  • 8
  • Are you sure they are not giving it at least a cursory review before approving? Like, have you put code in that obviously shouldn't have gone in and they approved anyway? – Underverse May 30 '19 at 13:41
  • 6
    You can try introducing an obvious ( but harmless ) bug into your code to see if they are actually reviewing the code. – sf02 May 30 '19 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 '19 at 13:45
  • 35
    I really don't want to introduce any bugs. I'd rather go a different route. – mkki May 30 '19 at 13:46
  • 1
    @mkki If there is a bug in the code, and it is traced back to this merge, then whomever is responsible for the system may come back at that point and ask if the process for merges was actually followed and for an explanation. – Underverse May 30 '19 at 13:49
  • 1
    @Underverse unfortunately, we haven't released any features to the public yet. So, I'd have to wait a few weeks still to even find a bug that can be traced back to a bad review. – mkki May 30 '19 at 13:58
  • @mkki By then it is possible that someone else checks your code out, makes a change, tests and merges it back in. If so, their testing may pick up anything your unit / integration tests missed. SDLC, Agile, or whatever you use, it always works the same way. Cover yourself, make sure you have done what you can, flag it with management, and move on. – Underverse May 30 '19 at 14:03
  • 3
    A frequent problem for companies doing code reviews, is to misunderstand their purpose and to think that seniors don’t need them. I would bring that up with your teammates. – jmoreno May 30 '19 at 22:58
  • 3
    This question should have been asked on Software Engineering SE. – Chris May 31 '19 at 05:41
  • 7
    @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. – ProgrammingLlama May 31 '19 at 07:14
  • 4
    Do you do retros to discuss what you could be doing better? Not that I'd have a problem with talking about code reviews at any time, but a retrospective might be a good opportunity to bring such things up. – JollyJoker May 31 '19 at 08:04
  • 1
    @JollyJoker indeed, it's literally what they're for! – BittermanAndy May 31 '19 at 11:44
  • 3
    @mkki completely understand you don't want to deliberately introduce a bug. How about a comment that says "if you spot this comment during code review, show it to mkki and he will give you $5"? Soon find out whether they're looking at it closely or not... – BittermanAndy May 31 '19 at 11:47
  • @mkki While it absolutely doesn't excuse skipping the review, 509 LOC across 10 files is kinda iffy. – lucasgcb May 31 '19 at 12:19
  • To me it seems the problem is that they approve so fast they clearly didn't review anything. An absence of comments is not really the problem. I suggest changing the title to better reflect the body of the question. (This isn't "moving goalposts" of the question since your question is the same, you simply give a better summary.) – Captain Man May 31 '19 at 14:47
  • 1
    What is your work organization process? Are you in Agile? Do you have sprints and retrospectives at the end of sprints? Is there any other scheduled ceremonies you have where you can bring up things to start/stop/continue doing, where this topic would be quite relevant? – Ellesedil May 31 '19 at 18:53
  • 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 '19 at 18:59
  • 2
    Question regarding the "I'm also the most senior dev on the team" comment - you are Senior in years I assume, but are you also senior in position or in some official form? If either of these are a 'yes', it drastically changes what you can do to change company culture, beyond the currently accepted 'just ask for comments' answer. – Zibbobz May 31 '19 at 20:12
  • Typical typical typical in my company. People don't say "can you review my PR?", they say "can you approve my PR?". I'm lucky I work in a small team that cares about code quality standards - we actually do the review and our PRs typically need to be updated 1-3 times before merging. – Andrejs Jun 02 '19 at 09:47
  • Do they do it with other's PRs as well? Do you yourself make PR comments? ASssuming you do, how the authors react then? – max630 Jun 02 '19 at 09:55
  • What is the official policy of the company? Are they expected to review your code? Or is it just that "you cannot approve your own pull request"? – Thorbjørn Ravn Andersen Jun 02 '19 at 18:01

14 Answers14

118

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.

Community
  • 1
SamGibson
  • 761
  • 2
  • 4
  • 8
  • It might be worth getting everyone together and asking "do we do code reviews, and if so, what's the process", or "we don't seem to do code reviews, should we start?". But maybe they don't see the need for them at all (hey, Agile is do what works for the team!) and have other processes for code quality after the event (I make no judgement over whether this is good or bad, that's not the question asked). – gbjbaanb May 30 '19 at 23:44
  • 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 '19 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 '19 at 08: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 '19 at 09:23
  • Pulling someone aside to talk about this will probably feel weird to that person no matter how you go about it. It would probably be better to wait for a more natural-feeling one-on-one, such as going for lunch/coffee, then bring it up as casual conversation. – aleppke May 31 '19 at 15:07
  • 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 '19 at 16:49
  • 3
    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 '19 at 17:03
50

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?

Underverse
  • 898
  • 2
  • 11
  • 21
  • 5
    PRs from other teams have tons of comments, good comments, not nit picks. – mkki May 30 '19 at 13:48
  • 2
    Okay, there's a problem, but it isn't your problem. You have no control over what others do, and each person has to do their job. I suggest adding this info to the original question - that your requests are different from others. – Underverse May 30 '19 at 13:50
  • @mkki By "other teams" do you mean people outside of your team who makes PRs? If so, it makes sense that members outside of your core are under heavier scrutiny. – Dan May 30 '19 at 16:38
  • 3
    It may also not be the problem of the person doing the approval. When you're resource-strapped, sometimes you have to choose good practices by their risks to make sure the team delivers. In the same way, you may be denied attending a long , relevanttraining workshop to improve your code shortly before a deadline. It's no different really. – Dannie May 31 '19 at 00:40
  • 103
    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 '19 at 11:13
  • 2
    @MarkAmery well said! It may not always be possible to make the world a better place, but it is what we should strive for. – BittermanAndy May 31 '19 at 11:48
  • 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 '19 at 13:29
  • 21
    @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 '19 at 14:06
  • 2
    @MarkAmery What you fail to see is that if you're so great at fixing dysfunction at your organization (you said you won't ignore dysfunction even if it wasn't your job responsibility), then why not share your experience at how you stopped said dysfunction instead of adding another voice? It'll be interesting to see how a fellow coworker who has no authority outside of being a member of a team hired for a specific purpose be able to change the organization for the better? – Dan May 31 '19 at 14:29
  • 10
    @Dan "It'll be interesting to see how a fellow coworker who has no authority outside of being a member of a team hired for a specific purpose be able to change the organization for the better?" - as step 1, by suggesting that people do things the way that you think they ought to be done, and telling them why. That's frequently enough. – Mark Amery May 31 '19 at 14:40
  • 3
    @MarkAmery It's unclear if you're talking from experience or trying to state a noble thought of how things should be. I do not believe you are speaking from experience as you stated none. Convincing people is hard and it's even harder when you have no authority to do so. – Dan May 31 '19 at 14:41
  • 23
    @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 '19 at 14:48
  • 1
    @MarkAmery "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." - Great.... so this experience of yours will be super great to share as an answer to this question. Since this seems to be a trivial issue (unremarkable as you call it since it occurs on a daily basis) why not create your own answer and share that experience where you managed to convince numerous people to make comments on your PR? It sounds like you're going to be of great help to the OP. – Dan May 31 '19 at 15:00
  • 23
    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. – GuilleOjeda May 31 '19 at 16:51
  • 4
    Being "The Guy" that reviews and approves pull requests, I've largely told my team, "Look, I'm going to make sure there aren't any conflicts and that if I pull the code into the target branch, it doesn't throw compiler errors. I'll skim the code, if it isn't too involved and make sure it looks like you're doing things in a good way, but for the most part, I may not necessarily know what it is you're trying to achieve. I'm just here to keep master stable." There are other reviewers (but I'm the only one that merges) and secondary review process, so no comments (on the PR) is a good thing. – Draco18s no longer trusts SE May 31 '19 at 17:01
  • @Draco18s Where there is an automated process for code quality, unit and integration tests, and a clear display of changes with diffs, sure. My answer here comes from direct experience of years of working with people who have a job who don't do it. A lot of what is in these comments is true, and this should be on SE.SO for the technical side. On the workplace side knowing how to work within a team and be a part of that team is just as important. – Underverse Jun 01 '19 at 07:34
  • 3
    @MarkAmery: 100% agreed, for this issue more than most there's no harm in asking or mentioning this to the rest of your team. You can bring it up as a discussion topic, like "hey, what do you guys usually do for code review before merging a pull request? I'm wondering if I could get a second pair of eyeballs on my code." Unlike some workplace.SE questions where doing nothing has some justification, there's no issue of sticking your nose into other people's business. This is about how they deal with you. At least this answer does suggest talking to people. – Peter Cordes Jun 01 '19 at 21:37
  • 2
    @Underverse And every team is different. I know that I'd love to be using Unit tests (but they're frustratingly difficult to do in Unity 3D: oh it can do them, but 80% of your unit test are going to involve interacting with UnityEngine which can only be done as a "runtime unit test" which is not fast and awful to work with) and something like Jenkins, but I don't know how to set up and run Jenkins. There's only one other senior dev besides myself, so I basically function as Jenkins. In any case, wasn't really commenting on the comments, more providing my own perspective. – Draco18s no longer trusts SE Jun 02 '19 at 03:18
  • @MarkAmery: Assume for a moment that you have no knowledge of whether you're doing the right thing or the wrong thing. But you do have a choice to be proactive or not. If you are proactive and right, great! But if you are wrong, being proactive only makes it worse. "Zealot" is a word with a negative connotation, even though it strictly doesn't refer to right or wrong, but rather to the passionate or proactive approach. So in short, not being overly proactive protects you from the possibility that you're wrong and will be considered overzealous and creating problems (even if well intentioned) – Flater Jun 03 '19 at 10:36
  • @MarkAmery: I agree that being proactive is the best way to get the best outcome. But not being proactive protects you from the worst outcome. Especially when in a situation where you can get chewed up based on your results (and not your intentions), it's safer to err on the side of not being proactive. I don't like it, but there are plenty of workplaces where the communal spirit is non-existent and people are barely able to cover their own asses. Your intention is noble but without group support just makes you a target for anyone who is willing to exploit your good will. – Flater Jun 03 '19 at 10:37
  • @MarkAmery Your comment is noble and I agree with you from an idealistic perspective. Unfortunately, boots on the ground, it is impractical. There are a host of companies, including ones I may have worked for, where pointing out flaws in the system or culture, even those that are obviously toxic and where an excellent solution might be trivially implemented, can get you thrown into hot water. At the end of the day, most people do not care about anything other than taking home their paycheck. The ultimate good of the company, the product or the team never crosses their mind. – Reverse Engineered Jun 30 '23 at 06:07
11

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

user168715
  • 756
  • 4
  • 8
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.

Kevin
  • 30,129
  • 9
  • 64
  • 108
  • 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 '19 at 13:59

  • you can't force them to change their opinion about what their job consists of

    Key word being "force" - you can definitely talk to your team and tell you them want more PR feedback and maybe examples of tone. For example I always clarify upfront if something is a blocker or just a nitpick, and then the author can always push back on a blocker with a discussion but can skip over nitpicks as a matter of preference

     – Josh G Aug 03 '23 at 19:55
8

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.)

wberry
  • 854
  • 7
  • 15
  • 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 '19 at 15:58
  • 2
    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 '19 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 '19 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 01 '19 at 01: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.

user1666620
  • 21,565
  • 12
  • 60
  • 80
  • I might have to reach out outside of my team to get this. – mkki May 30 '19 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 '19 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 '19 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 '19 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.

dbeer
  • 11,944
  • 8
  • 31
  • 39
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.)

user3067860
  • 2,522
  • 15
  • 15
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.

GuilleOjeda
  • 1,292
  • 9
  • 12
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.

gnasher729
  • 169,032
  • 78
  • 316
  • 508
  • 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 '19 at 04: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.
Egor
  • 4,337
  • 2
  • 16
  • 21
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.

Adam Smith
  • 111
  • 2
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.

Wayne Werner
  • 745
  • 5
  • 10
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.

Level River St
  • 1,663
  • 9
  • 13