6

I work on a small team, comprised of my manager (project lead), me, a coworker, and a senior full-stack engineer who isn't really part of the project, but has been helping us build an internal frontend. I have written about 95% of the codebase, with the last 5% from my coworker, who splits his time elsewhere.

Technically, we are supposed to do code review. I think code review is a great concept and really want feedback. My manager heartily espouses the benefits of code review, and I've often overheard him tell other teams how important it is.

Somewhat ironically, however, my manager drags his feet on doing them. He takes the better part of the week to merge my PR (we're talking maybe 40-50 lines of code), and leaves no comments. More often than not, I have to remind him a pull request is still pending, and he'll just approve it literally right then and there so he's "not a blocker". Totally understandable as as a one-off under a time crunch, not so much when it's basically every time I make a PR.

I often make business/design decisions in my code, and I'm not perfect--feedback would be great, especially since on paper he controls the product from a technical perspective. I often end up asking the senior fullstack engineer guy to review instead, which he always does within hours. However, he isn't really part of the team, nor does he have any stake/interest in/insight into the larger product, so I feel bad about repeatedly asking him.

I really like my manager in most respects; my manager is very responsive otherwise and flexible about making time for other things. It's just like he has an aversion to reviewing code for some reason. Or so I thought. Recently, I learned my manager is actively involved in code review for another, new project/team he manages. He often asks me for feedback in one-on-ones on how he could do better, and I'd like to point out I don't feel like I'm able to solicit meaningful code reviews from him.

Would it be out-of-line to broach this in our next 1-on-1 with something like the below?

One thing I’d really appreciate getting—and honestly don’t feel like I’ve gotten—is critical feedback on my work. I don’t write perfect code, and there is sometimes business logic I might not be thinking of. Often when I assign you as a reviewer, however, I don’t get any feedback, and it seems like it gets approved right on the spot without any review when I bring up the fact that the pull request is still pending. So… I guess my question is, how do you want me to handle code reviews? Is there something I can be doing differently or to make it easier to review? I’m of course happy to ask "Chris" to review instead, but I feel a little awkward doing so repeatedly when he’s technically not intimately involved in this product.

(For what it's worth, not a receptivity-to-feedback thing--my manager has commented before how receptive I am to feedback. I've considered perhaps my manager is reluctant to review my code but will do so for his new team because they're more junior, whereas my programming ability well eclipses my manager's. Not saying this with any conceit, just reality. Maybe he drags his feet because he doesn't feel like he has anything to add? I'm not sure, but I still would like his thoughts on design nonetheless...)

edit: Just to add--collectively, there are about 5 people spread across 2 projects my manager leads, including me. So I'm not really competing against a lot of other people in terms of PRs. I'd definitely get the delay if there were a lot of people across my manager's two teams, though! I guess it's more of a problem for me because I don't get the sense it is driven by lack of time (e.g., he will comment on how free/quiet the past few days have been, yet during that time I'll have reminded him that my PR is still outstanding several times. "I'll take a look this afternoon" and doesn't).

2
  • Do your other coworkers (the one coding 5% and the full-stack engineer) also review your code? If not, would that be a way to distribute code review duties across more people?
    – Llewellyn
    Jun 1, 2019 at 17:41
  • Yep... often I just ask the full-stack engineer exclusively, since he's thorough. I am not releasing PRs all that often, though, maybe once a week at most (in large part because much of my time is not spent coding, but investigating upstream data quality issues). Not releasing a nonstop torrent or anything.
    – kodachrome
    Jun 1, 2019 at 20:00

6 Answers 6

5

It is absolutely the right thing to do, and what you've written sounds like a really good way to approach him. It is quite common as a more senior dev to find people just assume you've done the right thing, and you could probably labour the point that we also make mistakes, and potentially larger ones in terms of consequences.

A lack of feedback is not only a way for things to slip through, it's also disheartening and you could point out that your boss's time spent doing deeper reviews is not only good for the team technically, it also builds bonds of communication and trust between you if you're able to talk/write in a frank and informed manner about the state of the code.

3

As a manager, I can understand this completely. And when I read the OP's quetions, I actually paused to consider that he or she might be someone on my team. :)

When I was managing a smaller team, I could review every almost every line of code and give quick feedback on PRs. Now that the team nearly doubled in size, I can't review as much as I used to. As a result of the team getting bigger, my time to actually review code (complicated by the fact that more PRs are now coming in faster than ever), is even less. I have more meetings to attend to. More weekly one-on-one meetings with everyone on the team, etc... Further, the team occasionally gets frustrated by the number of PRs they are expected to review as well. I think it's an O(N²) problem.

I would advocate for this:

  • Don't let your manager be the bottleneck for code flowing in the system. Ask him to come up with a PR review plan such that all code gets reviewed by appropriate area owner. Or at the very least - by at least one other engineer.

  • Area owners are expected to review code. Just like if your manager was touching your code, he would be wise to get your input as well.

  • Automate as much of this as you can. Many git based project management tools will automatically assign PR responsibilities to the appropriate engineer based on area ownership. Everyone can log in and see the dashboard of pending PRs assigned to them for review.

  • Some reasonable rule about getting at least one or two sign-offs and all feedback addressed before enabling the commit to get pushed to Master. Again, the tools can enforce this.

1
  • Agreed. I'd completely remove the manager from the review process for these reasons. Assuming the manager isn't trying to write code in addition to doing their actual job (i.e. management), they shouldn't be part of the process for reviewing it either. With that said, it would be to the manager's benefit to (informally) review their staff's work every now and then, so they can monitor and assess their staff's performance. But that should probably be a separate thing entirely to the team's own code review / pull request process. Jun 1, 2019 at 21:40
1

Your own suggestion on how to bring this up looks completely reasonable and wouldn't be out of line at all. If your manager is responsive and flexible as you describe and furthermore sees the value in code reviews, they will most probably welcome this feedback and will act on it or at least have a constructive dialog into how to improve this situation.

A few points to consider:

More often than not, I have to remind him a pull request is still pending.

Some code review systems are really bad with notifications, so it's quite possible your manager isn't really realizing that they have code to review. Particularly if they're watching all PRs in the project (to keep a high level view of where things are and track general progress), it's likely they're missing the PR review requests.

Also, a manager is typically busy managing their teams (and so should they be), so perhaps code reviews really shouldn't be in their top priority?

I often make business/design decisions in my code.

Perhaps this is part of the problem? If you'd like to get your manager's validation on these decisions or, better yet, have a discussion on the right approach before even writing code, maybe having a meeting, perhaps on a whiteboard, would be better than using code reviews for that purpose?

It's understandable that your manager is the one responsible for the design of the system, but not necessarily the decisions on the code itself.

My manager is reluctant to review my code but will do so for his new team because they're more junior, whereas my programming ability well eclipses my manager's.

To my previous point, if the design and business logic are agreed upon, then code reviews can and should focus on the best way to implement that and it becomes more about good coding practices, readability and expressiveness.

So perhaps "Chris" is indeed the right person to review your code, even if he's not that familiar with the system? If you feel like he's bringing useful suggestions in that area (and occasionally flagging something that looks odd to him and might uncover bugs), perhaps that's the best arrangement? Since he reports to your manager, it's something that could be easily formalized, as long as all parties are happy with it.

Another possibility is someone from another team altogether to review your PRs, with the intent of making sure the code is easy to grasp for someone not intimately familiar with the project, which is really helpful when new members are joining the team.

1

I think you'd be fine saying that. Though I'm wondering if this is the best system in the first place. If you mostly want his help with business logic, could you write up a quick design document about your business decisions and have your manager review that instead? It may be less bandwidth on them to understand, it will help you separate "business" ideas from "code" ideas, and it's good practice for when someone asks you later why you decided to do something a certain way.

0

What you need is engagement.

What you're getting is rubber stamping.

They way to fix this is to read your audience. 40-50 lines of code might seem trivial to you but that doesn't mean it's trivial to him. Break down your reviews and simplify your code until your reviewer can clearly understand it. Engage with him early while the problem is still fully in your mind. Tell the story so it's fully in his. Pair program if you can get him to sit down with you.

Get him to make even the tiniest suggestion and then show him how quickly you can adopt it. Make him feel like this can be quick and fun. I keep toys at my desk to keep people entertained while I type for exactly this reason.

Do that, build some trust, and you should see the feedback start to become meaningful.

6
  • 1
    I've gotten after folks on my team for PRs containing 40-50 files containing changes, and have asked them to re-submit into more discrete and manageable chunks. But 40-50 lines is a trivial PR. I'm guessing the manager is tied up with other responsibilities - it happens.
    – selbie
    Jun 1, 2019 at 18:02
  • @selbie this is true but you work with the team you have. Not the one you want. This is a best effort strategy. If it works that number will grow. If not you have to use other people. But someone that won't review code shouldn't be doing the pull requests. Without the review it's a pointless ceremony. Jun 1, 2019 at 18:10
  • There are positive aspects to this answer (not least: "you need engagement"), but I can't help but feel that breaking down changes to make them easier to review rather defeats the point. Yes, a review of a 1-5 line change will be quick, but will it be meaningful? Will it reassure the OP that the boss is properly monitoring his progress, and will it demonstrate to the manager the quality of work the OP is doing? It seems unlikely, so is prioritising the wrong thing. (Also... know your audience. Personally I don't want or expect code reviews to be "fun" or involve toys. Others may differ). Jun 1, 2019 at 20:07
  • @BittermanAndy every second that goes by between when I write the code and someone reviews it exponentially increases the cost to change it because my memory of the code and the problem has a half life. If you catch me before I'm thinking about something else a review is fun. After that it's a pain that everyone will fight. Time is not your friend here. It should be fun or you're doing it wrong. Jun 1, 2019 at 20:18
  • OK, but that's not really addressing what I was getting at... so, you write some code, 5-10 lines maybe. While it's still fresh in your memory, I come over to review it. We have fun, laughter and smiles. I confirm that your trivial change looks sound. The review worked, hurray! But have we done anything useful? If your code has problems, it'll (hopefully!) be in something non-trivial. If I'm interested in knowing how well you are performing, I need to see something non-trivial. So neither of us have learned anything. Code reviews for the sake of having a code review (fun or not) aren't useful. Jun 1, 2019 at 20:26
0

There are a few things to consider here that I have not seen mentioned in other answers.

Do you really need your manager to review your code? You mention that others do when asked, even if they are on other teams. From the perspective of noticing bugs or other issues in your code (one objective of code reviews), this seems like it should be enough.

Does your manager not want to review your code, or does he think he doesn't need to? You mention that he is performing code reviews for other teams, but that he rubber-stamps yours. It sounds like you have an otherwise-good working relationship with him, so it seems unlikely that he doesn't care. Perhaps he simply trusts you enough that reviewing your code is at the bottom of his priority list - perhaps he has other things on his mind. It is noble to recognise your own imperfections and want a review anyway, but your wants may not override his needs.

Is your manager capable of effectively reviewing your code? If his technical skills are "rusty" or he isn't familiar with the technologies you're using, he may feel he has nothing to contribute. He may even feel embarrassed or otherwise reluctant to admit that.

What are your manager's priorities, and how critical is your project? If your manager is getting heat from his manager about progress on another project, yours may get neglected - there are only so many hours in the day. If this is the case, it's worth working out whether this is a temporary or permanent situation - if it's permanent, you might have bigger problems.

Most importantly, what do you actually feel is missing here? Code reviews have several purported benefits, but I get the impression that the review process itself is mostly working in your team - it's just the fact that (specifically) your manager isn't doing them for you that bothers you. Is the problem then, really, not with the reviews per se, but your visibility to your manager, to make sure that he knows how well you are performing? Could that be achieved in some other way?

My suggestion is that you ask yourself those questions and answer them in your own mind as well as you can. Some of them you may not be able to answer yet. In your next 1:1, ask your manager to answer the same questions, especially the ones you don't think you know the answer to. Don't open with a criticism, even a gentle one as in your suggestion. Seek to thoroughly understand the situation, from both your and your manager's points of view.

Once you have that understanding, then you can explain your thoughts and, if appropriate, request action from your manager - you will have a much clearer idea of what you want that action to be, because you might decide that your manager carrying out the review isn't actually the outcome you really need. As things stand, it sounds as though you have not been shown the whole picture. Try to see as much of it as you can before asking for specific changes.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .