5

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

  • 11
    Isn't pair programming the most extreme form of in-progress review? The problem with your architect's review is that it is being done without the context of what was planned to be done today and what is planned for tomorrow. – Patricia Shanahan Nov 12 '16 at 23:19
  • 3
    What sort of things are we talking about regarding "stuff that would have been taken care of later"? If its features, fair enough, but if its comments, unit tests and other similar things, then your architect has a point - code shouldnt be checked in without corresponding comments and tests already in place, those arent the sort of thing you come back to. This question cant be answered without a lot more detail. – Moo Nov 12 '16 at 23:43
  • 2
    @programmer nope, you still do code reviews in pair programming - part of the point of code reviews is that the reviewer is divorced from the point of the committed code, so they are reviewing it in almost isolation, not part of a solution to a problem. This way the reviewer can concentrate on code quality rather than be blinded by the solution. – Moo Nov 12 '16 at 23:58
  • 3
    @programmer add that to the question, comment have a habit of disappearing. Security is also not a "do it later" job, nor is configuration - why would you need to visit any environment, why dont you have an automated deployment system and continuous integration system? – Moo Nov 13 '16 at 0:05
  • 3
    @Moo This is your opinion, but I really disagree. I think it's perfectly okay to push work in progress to a private branch at the end of the day, even if it's not compiling. This is a back-up mechanism and has nothing to do with how the commits will look like in the branch once the code is ready. See for instance softwareengineering.stackexchange.com/questions/246328/… – Étienne Reinstate Monica Nov 17 '16 at 11:35
16

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.

  • 6
    Actually my team will often raise a PR before ready and use github's labelling system to add "WIP" tag. This is an invitation for design reviews . . . and design reviews are IMO appreciated more before all the code is written. Even better, if you have time, is to research and present high level design before starting to code. But some projects IME fall into "too small to have formal design up-front, but large enough to impact system design in a way that would benefit from being reviewed". Difference to OP's situation is this situation is mutually agreed upon per project. – Neil Slater Nov 14 '16 at 11:55
  • As I said, code reviews are expected and appreciated after a pull request. In this case, the reviewer found a huge list of "problems" that were due to the job not being completed yet. Pointless and wasting everybody's time. – gnasher729 Nov 14 '16 at 20:02
  • Yes, and if the architect is reviewing lines of code, not the design/use of technology etc, then they are not looking at what I'd consider architect stuff in any case. I just wanted to point out there are some reasonable motivations for reviewing incomplete code - and those are compatible with a software architect's role. Finding typos in incomplete code is a waste of time. Finding major problems with data model, incorrect split of tasks between system components etc, can be better with early feedback because it saves time. – Neil Slater Nov 14 '16 at 20:08
  • Of course, but it's only applicable if the author of the code is seeking for help. – Jonast92 Nov 15 '16 at 16:28
  • 1
    As a follow up on this, pass along to your director that the Architect is doing a code review of work in progress and wasting everyones time. You can mention that while the points he made were valid, they were also points you already knew about and were planning on finishing up as you progressed. Reiterate that reviewing work in progress for nitpicks is a waste of everyone's time. Don't open any doors like accepting architectural review points, you do not feel code reviewing prior to the pull request or whatever your process dicates is helpful. – Bill Leeper Nov 15 '16 at 21:16
12

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.

  • Maybe also the architect has the task to keep code quality high and the OP has turned in poor code before. – Robert Nov 16 '16 at 15:05
  • @Robert ...and maybe he is a nosy busybody whose idea of progress is forcing everybody in the company to use 2-space indents. – Socrates Nov 16 '16 at 15:38
6

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.

2

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.

  • 4
    The simple fact is that reviewing incomplete code is nonsense. It's like a car mechanic who knows he has to do steps a, b, c, d, e and f to fix your brakes, and after he's done a, b and c you review his work and complain he's done an awful job because d, e and f are missing. – gnasher729 Nov 13 '16 at 8:50
2

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

1

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?

  • I wish I could but he already knows it's in progress code. In the daily status meeting he said he knew it was in progress but thinks reviewing it would be helpful. – programmer Nov 14 '16 at 13:06
  • @programmer - Is it an option to just semi-ignore (i.e. not ignore, but not prioritise too highly) his comments and if he complains just nicely tell him something like, "Yeah I looked through your comments - As I said the code is still in progress, so some of that we were going to do anyway. Don't worry, we'll get to your feedback". – colmde Nov 15 '16 at 8:34
-1

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.

  • 2
    While true, this is not an answer. – user42272 Nov 13 '16 at 23:35
-2

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

  • 2
    You can still review all the individual commits even if you wait until the whole feature is done. Of course, that's not usually very useful because many people will intentionally first make it work in an ugly way and then spend some time to fix all the coding conventions, so the final product is ready for review. If OP is like that, this really is a waste of time because he's aware that he's making a mess; it's because he isn't done yet. – Erik Nov 16 '16 at 6:42
  • @Erik Sorry, commit is commit. Clean up before you commit. – Robert Nov 16 '16 at 15:04
  • I'll stick to "different opinions on when to commit". – Erik Nov 16 '16 at 15:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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