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

programmer
  • 182
  • 1
  • 5
  • 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. –  Nov 12 '16 at 23:43
  • 1
    I also thought that pair programming code didn't need typical code reviews since there's already an extra set of eyes on the code. I think you're right that he needed that extra context of what was planned. This dev shop is new to doing code reviews (as am I). I just don't know how to approach this. I feel having to submit a code review each day and providing all our "to-do"s is adding unnecessary busy work that isn't adding value. – programmer Nov 12 '16 at 23:53
  • 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. –  Nov 12 '16 at 23:58
  • 2
    @Moo, I do test first development and for this particular commit we had tests for each method. The code reviewer's comments were not regarding 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. – programmer Nov 13 '16 at 00:02
  • 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? –  Nov 13 '16 at 00:05
  • @Moo, I'm not really challenging doing a code review on pair programmed code. I'm just wondering about the merit of doing code reviews on code that's in-progress. This code is all being committed to a dev branch, which isn't production ready or even assumed to be ready for production yet this review was as if it should have been ready for production. – programmer Nov 13 '16 at 00:07
  • @Moo, are you saying code should not be committed to source control until it is production ready? – programmer Nov 13 '16 at 00:09
  • @programmer Im saying a commit that intentionally needs to be revisited is not a commit that I would make. I work in small features - each major feature broken down into smaller blocks, each of which builds on the last bit also each of which is fully and 100% production ready in itself. If I need to save some work in progress, then it gets stashed, not committed. The way I work, code reviews can be done at any time and make sense at any time. Incomplete code should not be committed to any branch. –  Nov 13 '16 at 00:13
  • @Moo, I don't have the power to break down the tasks but thanks for the perspective. I think I'll start doing exactly what you're talking about regarding commits. What if the architect wants to start reviewing our stashed code? Because that's essentially what he wants to do. – programmer Nov 13 '16 at 00:19
  • 1
    @programmer I dont think he will - but if he does, you can tell him the code is stashed and therefore not intended to be a final version. If he criticises the code even then, find a better job because hes just looking for issues. Stashes are intended to be a scratch pad that can be thrown away at any time, commits are intended to be history in the making. –  Nov 13 '16 at 00:24
  • 1
    @programmer it does sound like you might have been committing too frequently, which sounds great to begin with but makes for poor version control - a commit should be meaningful. One thing you might want to look at doing if you still want to commit often is simply not push the branch to the remote - wait until you have a finished feature or sub-feature and then do a roll up or rebase commit, which wipes out your incremental commits and gives you one feature commit that you can then push. –  Nov 13 '16 at 00:27
  • 1
    That is CRAZY. That is like saying the meat is not done when you are still cooking it. – paparazzo Nov 13 '16 at 00:30
  • @Moo, Haha, yeah I commit multiple times a day. An old coworker mentioned combining commits. It was one of those things I had in the back of my mind to start doing but just didn't get to it yet. I'm going to turn over a new leaf Monday with source control. :) – programmer Nov 13 '16 at 00:48
  • In my company, some people ara adamant refusing "simple" code reviews, it seems your company is in the other side of the excess. – gazzz0x2z Nov 13 '16 at 08:33
  • @programmer you don't have to break down "tasks" to break down code into bitesize commits. You can make tested, integrated and in-production commits that don't actually have an effect on the system's behaviour yet and that is a very good thing - much better than integrating a big feature branch, pulling the live deployment lever and hoping for the best (hence continuous delivery). – Ant P Nov 14 '16 at 14:55
  • 2
    @Moo: You can commit to your own branch as often as you like. Because your own branch is work in progress, and nobody should care about commits to it. In this case, because two developers are working on the same feature, frequent commits to their shared branch are done for communication between these two developers. Still nobody else should care. In the end the complete branch is reviewed - when it is complete. – gnasher729 Nov 16 '16 at 10:08
  • @gnasher729 I disagree, if you push that branch then you are making a declaration that anyone else is free to study it at will. If you want to keep the branch private, don't push it to the main repository. Many people seem to use source control as a dumping ground for code in any state - this is something that shouldnt happen, even when multiple devs are working on the same codebase. Push features, not code. If your feature is incomplete, don't push it to the main repo, stash it. –  Nov 16 '16 at 10:13
  • Can you edit your question instead of adding comments here? It's really hard to get the big picture if half of your information is not shown by default. – Robert Nov 16 '16 at 15:07
  • 1
    @Moo I strongly disagree. Stashing code won't protect you against hard disk failures, pushing will. – Étienne Nov 17 '16 at 08:15
  • @Étienne then push small complete and self contained features - the ops issue is that they were committing incomplete code and getting called on it. This indicates that the chunks they have broken the work down into are too monolithic, they need to be broken down more. The fact that they admit in these comments that configuration and authorization were some of the missing things questioned does support this for me. –  Nov 17 '16 at 10:06
  • 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 http://softwareengineering.stackexchange.com/questions/246328/how-can-a-manager-ensure-developers-are-pushing-up-to-the-origin-every-day – Étienne Nov 17 '16 at 11:35

8 Answers8

17

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.

gnasher729
  • 169,032
  • 78
  • 316
  • 508
  • 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.

Socrates
  • 7,227
  • 1
  • 18
  • 23
  • 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.

Burhan Khalid
  • 3,714
  • 1
  • 19
  • 24
  • 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 08: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).

Thomas Cox
  • 1,816
  • 10
  • 18
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?

komodosp
  • 5,720
  • 17
  • 26
  • 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". – komodosp Nov 15 '16 at 08: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.

Xavier J
  • 42,848
  • 10
  • 86
  • 146
-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).

Robert
  • 241
  • 5
  • 15
  • 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 06: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