Newly promoted software architect wants to do code reviews of code in progress

Question

I work as a software engineer with a team of 12 others. We recently had a Sr dev get promoted to architect. He recently got a talking to by the software director for being unprofessional, aggressive, and belittling to several team members. Afterwards the software director met with us individually to try to get us to work together and try to make it work.

Just days later the architect took a special interest in an in-progress feature (about a 3 day task) that another engineer and I are pair programming on. In our daily status update meeting he convinced my project manager to allow him to do a code review for each daily commit of the in-progress code.

We're using our source control system to do the code review without having to all be at an in-person meeting. So my pair partner and I received the first review back with over 20 comments on stuff that would have been taken care of had he waited to do the review after the task was done.

I personally never heard of doing code reviews on in-progress code. It seems wasteful to me.

My concern is he might try to use these reviews as fodder to undermine my skill and cause trouble. How do I professionally respond to the comments saying that my partner and I already planned the additional things he's pointing out but we're deferring them until later in the process?

EDIT: To clarify and add a bit more details, the code reviewer's comments were not regarding adding tests or code comments. These were things like adding authorization checks to api endpoints and adding staging and production configurations. We intentionally left those off to have easier access to them during development. Plus config settings change during dev and constantly visiting stage and prod for every config setting change is a waste of time

Answer 1

Your so-called "architect" is ridiculous. Reviewing work in progress, unless requested by the person doing that work, is a waste of everyones time. My answer if someone wanted to review my work in progress (and there is nobody around me doing something stupid like that) would be to tell them "review all you want, I don't care because it's work in progress, meanwhile I'll have a conversation with your manager about wasting my and your time".

Code reviews are expected and appreciated after a pull request. When the code is complete. Not at any time before between.

Answer 2

Sounds like the architect has way too much time on his hands.

When people make worthless, editorial comments about my code I ignore them or perhaps respond with a perfunctory: "Thanks for the great ideas!", then delete their email.

I suggest you do the same. Life is too short to worry about stuff that is non-productive like that.

Answer 3

Really, he is just making a mistake, and I would let him make this mistake so you can focus on having a good relationship with him in a week or two.

"Okay, I was planning on doing most of these anyway so I'll take another look in a day or two. In the future you might want to just do review at the end, but if you want to take a look early I guess there's no harm."

I'm not sure how he would respond to that but I'm pretty sure it's a good stakeout for now.

Answer 4

You have two issues as I see it:

1. Your supervisor doesn't understand pair programming; as code reviews are a part of the way pair programming works.

2. He/she is not aware of the backlog, or the sprint, or however you are managing the feature set that you are developing.

A possible third issue could be, he/she is doing this peer review to counter the earlier criticism from their superiors.

In any case, this isn't good for you or your pair programming mate.

I would suggest sitting with your pair programming mate and this person, explain to them how you plan on developing the feature, welcome their code reviews, but explain when they are most beneficial to you. This could be as simple as:

"It would be great for us if you could provide the code reviews for yesterday's pulls the next morning so we can make sure we can address any concerns immediately that day."

If you are careful, you will avoid coming off as someone that doesn't take criticism well (a common complaint, especially in software development); and at the same time make your supervisor aware of how best they can help you.

Answer 5

Your newly promoted person is suffering from a common error, "trying to add too much value." He doesn't really have his feet under him. He's looking for ways to be helpful but his helping is hurting.

See if you can manage to confront him with love. "Dude, I love it that you care about code quality. But when you review code that's in the middle of being pair-programmed, you flag things we were going to fix anyway. That wastes your time and talent. If you did it with someone who didn't know you as well as I do, it might harm your relationship with them. Can I offer you some advice?"

(Wait for him to say yes.)

"You were promoted for a reason. Get clear on what you can uniquely do for us in your new role. It's not code reviews like this. It's something. What?

"Most newly promoted people struggle to let go of their old activities. Don't feel bad. Just let it go and focus on this new role.

"I know an experienced architect, named __. I'd be happy to introduce you. He told me he got a ton of value when he was new, by talking with more experienced folks. I know he'd love to give back."

(Okay, don't say this if you don't know __. ;-) You may need to do a bit of homework.)

In sum, he sounds like he's struggling to find the best way to fill his new role. Give him compassion and help him (appropriately).

Answer 6

Can you just reply with an email.

Hi [Architect]

Thanks for your feedback. FYI this work is still in progress and so it's not fit for review just yet.

We will let you know when it's complete

Thanks, programmer

and leave it at that?

Answer 7

He's not an architect. He's a control freak. He may as well tell all the devs to not write any code unless he can pair with them on a personal basis.

This approach is completely counterproductive. Escalate this to your boss.

Answer 8

I'd like to take a different point on this. Unfortunately your question did not have some details, so I'll make some assumptions.

First, when you commit to the VCS, you say that you have reached a point in development where the committed code is usable by others, especially if you commit to trunk / master. A feature branch would be a bit different, but still: once you commit, you can expect people to take a look.

It is much easier to review code in smaller chunks. Even if not everything is complete yet, you can review what's there and provide feedback. This is especially important if the code seems to go in the wrong direction or there is a lot of gold plating.

As a team lead, I've been struggling with a developer who consistently turned in poor code: lack of unit tests, missing Javadoc, unneeded complexity, dead code. The bigger the code chunks I had to review, the harder it was to understand it, and the harder it was to track all the comments I made to see if the developer addressed them. Unfortunately, you don't say anything about you and your pair being junior or senior, or whether you and the architect had disagreements on code quality before.

You also don't say what comments you get in a code review. Missing tests? Poor naming? Complicated code? Missing documentation? Not following the company coding standard? These are IMHO all valid review comments (no mater if the feature is complete yet), and if you were not addressing them over time, I'd fire you for continuing failure to perform (after several warnings, of course).

If you did already plan to do the things the architect pointed out, take that as a sign that you're on the right way.

Finally, let's not rush to conclusions: you said this was a 3-day task. Reviewing three days worth of code hardly makes the architect a control freak or micro manager. Come back if this is still going on after 4 weeks and you haven't been fired (see above).