61

I was in a performance review recently and my team leader, who is a non-technical person was going through the usual: what do you feel has gone well so far this year, what do you think could have gone better, etc.

I was then completely caught off guard in that my team leader said they had gone back through our version control history (presumably with the aid of a technical person in the team) and was looking through all of our previous pull requests for this performance review period to see what comments had been left by our peers on each of our pull requests. As a result, my team lead said that across a number of my PR's that similar comments on fixes/improvements had been identified and as a result, they were marking my performance for this period as needing improvement. They admitted they hadn't read the code itself as it wouldn't have meant anything to them, but by reading the comments which had been left by my peers, they could make that judgment call.

For information, my team lead is very new to the management role but has been in the team longer than I have (which has been a couple of years).

This is the first time in my career that my performance as a developer has been called into question based on this criteria, I was wondering if this was commonplace outside of my current place of employment, or is this a bit of a red flag that something else may be going on?

2
  • 1
    Comments are not for extended discussion; this conversation has been moved to chat.
    – Kilisi
    Jul 8 at 0:59
  • 1
    "similar comments on fixes/improvements had been identified" Can you refine this statement? This is a very wide array of possibilities. If people have to ask you to use proper capitalization for months without improvement (to take an extreme example), the manager's interpretation of the comments isn't wrong.
    – Flater
    Jul 8 at 11:57

13 Answers 13

89

That entirely depends.

If they just sat there and looked at comments without understanding them, then it certainly is a poor indicator and I have never heard of it.

If they actually understood the comments and saw a specific recurring problem with your code, that would be a pretty good indicator.

So with this "improvement" they want you to do, did they give you a specific goal what to improve when coding? Or did they just say you need fewer of those comments?

One would be a good plan, the other would be totally pointless.

12
  • 13
    Relating to "the other:" commitstrip.com/en/2021/04/09/…? Jul 6 at 9:48
  • 10
    Absolutely agree with the point that it makes no sense, if they didn't give you a list of issues that are recurringly pointed out in your PRs. I'd also like to point out that recurring comments can also be part of "style wars" - which would be absolutely no indicator of lacking performance but an indicator for a manager to start managing.
    – Fildor
    Jul 6 at 11:56
  • 6
    @Fildor re "style wars" then a manager pointing out that the committer needs to stop doing this is managing and I would suyggest does show performance issue if the committer is working against the rest of the team (Basically without more details of what the issues were and what the requested imporovements are we cannot say if the manager is wrong)
    – mmmmmm
    Jul 6 at 12:40
  • 5
    "Style wars" comments very much depend on whether it's someone repeatedly not following the style the rest of the team follow. If you're consistently breaking that style, rightly or wrongly, arguably it's a performance issue. Jul 6 at 12:40
  • 10
    "Style wars" definitely lead to a performance issues. Have a written standard and everyone follow it. Don't waste time arguing about carriage returns and white space
    – Kevin
    Jul 6 at 12:58
35

When one needs to build estimation on an employee, it should consist of several factors. Not only on one thing.

For example:

  • You can do all your tasks at 1/10 of the time, but yell at people in the office.
  • You can plan amazing systems that will never fail, but when you write code you are always using O(n^2) complexity.

So to your question - The comments in the PRs should be one part of the overall review. Maybe you constantly get the same comments again and again, causing your teammates frustration? "Johny is always writing code like in the 70's and not using the latest features"

So "yes" - PR comments can give some information about your performance, BUT it should blend with other parameters.

1
  • Exactly, I'd worry more about what my peers think of me as a team member rather than what that manager had to say. Maybe find a couple PRs with interesting comments and ask your peers why they wrote that and how can you work towards improving that.
    – Juan
    Jul 8 at 0:58
17

I'm pretty appalled by this. When I make technical comments on someone's work, I propose improvements that are within the capability of the relevant person to fix, and the more capable they are, the higher the standards I'm likely to apply when reviewing their work.

As a manager, I would not be concerned with the comments other people were making on your work - I would be much more interested in how you respond to those comments.

2
  • 1
    Yes this is not a review but really just criticisms from an unqualified source. A review would also entail some ideas of how the problem could solved
    – Neil Meyer
    Jul 7 at 17:14
  • The biggest thing I see wrong with the review strategy is it might fall into the trap of punishing ambition. Hard, important changes might involve mistakes; specialists/gurus in the thing that nobody else knows trying to coach somebody might look like stubbornness or incompetence.
    – jrh
    Jul 9 at 0:32
12

I adjust my comments to fit the skill of the code's author, so just the presence of comments is not an indicator of their competence. It's an indicator of a reviewer that wants you to grow. There are also certain things that are easy for anyone to occasionally forget. Hopefully, your team leader understands that.

However, I can also imagine a conversation like the following:

"Can you tell me about Echo Bravo's technical skills?"

"To be honest, it has felt like it has been taking an excessive amount of repetition to see improvement in certain areas."

"Do you have an example? I prefer to base performance reviews on something objective."

"Sure. Here are some pull request comments that show what I was talking about."

Of course, it could also just be a non-technical team leader still finding his feet. I'm just saying it's not unusual to cite objective evidence on a less-than-favorable performance review.

8

This is utterly ridiculous and shameful.

Imagine every single bit of work that your team leader does was reviewed according to strict standards like yours. How many comments would you be able to leave that imply he needs improvement? Just check his last ten emails to you. Is the English in them perfect? No spelling mistakes whatsoever, nothing missing, nothing misleading, nothing unclear in his communication at all? I very much doubt it.

Now if I had reviewed your code and left the comments... First, there will be bugs found. That's one of the most important parts of a code review. That's normal. Unless you work in the aerospace industry where bugs are unacceptable and people don't write more than 17 lines of code a day on average, it's normal. There's a point where finding bugs in a code review is more effective than trying to write code without bugs (and it keeps the reviewer on their toes). It's teamwork. Your "team leader" should know what "team work" is, right? Apparently he doesn't.

Second, there will be suggestions. I will tell you "I would do XXX instead of YYY". That's a suggestion. You don't have to take it. Your code is perfectly fine, but I would have done it differently. You can keep that suggestion in mind and use it the next time, but it's in no way a negative.

Third, lots of people make suggestion that are outside the task that you had to accomplish. Critisising you for that is stupid.

So if this happened to you, based on my code reviews, I'd need a very serious one-to-one with that team leader, and if I can't get him to understand, then every single code review for you in the next year will contain just one comment "Your code is absolutely perfect, the best that I have ever seen".

(Forward his last twenty emails from him to me, and I bet I can rip huge holes into every single one of them. And what he did to you would get a "needs to learn the basics of his job before he can start improving" from me).

2
  • 1
    I'm surprised I had to scroll so much to see someone point out the obvious: had the manager understood anything about the work under review, there would be no need to use PR comments as the source. The actual work would be used in that case. For all the manager knows (nothing), OP's coworkers could be just pulling some light-hearted joke.
    – Ramon Melo
    Jul 7 at 18:03
  • 2
    Actually depending on what the comments are it could be totally reasonable... If you had told somebody 10 times to not do basic mistake in one month and they kept on doing it that is a pretty big problem. Jul 7 at 19:36
5

I have never heard of this specific one, and it makes little sense, but this is a common enough issue when non-technical people are looking for any metric to base something on.

is this a bit of a red flag that something else maybe going on?

I would think it unlikely unless there are other factors that aren't in your question. Having said that it's quite possible someone else pointed this out to be looked at.

2
  • This - you can assume incompetence on part of your manager, but assuming they're not incompetent (having proven themselves being on the team for a number of years), it's likely that this is an issue some of your peers have raised and your team lead just browsed through the comments to confirm that piece of feedback.
    – scrwtp
    Jul 6 at 10:52
  • Any non-technical person is incompetent to manage developers, in my opinion. It's just not a good idea at all. I've had both types of manager and the difference is like night and day. Jul 8 at 7:56
4

This sounds rather similar to scenarios I have encountered with peers a few times over my career. I'll give a narrative of what may have happened from a third party perspective.

Scenario

Occasionally a co-worker comes along who seems quite capable, but is rather resistant to learning new things or adapting to team standards. Other team members will leave similar comments on pull requests routinely. e.g.: "Only one class per file." or "We do not use Hungarian notation. Please stop prefacing your variable names with 'str'."

More experienced team members will eventually notice that the co-worker is not showing an attempt to learn from the comments repeatedly posted on pull requests. When they have a 1-on-1 with the manager they will likely mention this frustration.

A good manager would then ask for examples and evidence of when and where this is happening. Once the manager sees empirical evidence that this is an ongoing problem, they will likely bring this issue up to the co-worker. Rather than risking drama and team friction by relating anecdotes from team-mates, presenting the empirical evidence and letting it speak for itself should be enough for the co-worker to be made aware of areas in which they can improve.

TLDR;

While it may be possible your manager is being more pro-active than most, it is at least as likely the problem was probably brought their attention by your peers.

Solution

When you receive feedback from your peers, take it to heart. If the feedback is of a general nature, such as compliance with team standards, or something which you receive more than a few times make a note of it. Finally, before posting your code for review, perform a review of your own code using your notes to anticipate that your team-mates are going to flag you for those violations yet again.

3

We also had similar practice in our company but reviewer is a technical person.

The thing is whenever developer generates the PR/MR then reviewer/maintainer of that project will review the changes and add their comment in relative MR. During the performance review time they will visit the provided feedback for generated MR and if there is repetitive feedback then it will be consider as performance issue. i.e. if developer is doing code which is vulnerable to SQL injection and if they are doing this mistake repetitively, Not doing proper server side validation and thus site is vulnerable to XSS etc.

If feedback was already provided at-least one to two time and if still developer is doing the same mistake then it will consider as performance issue.

For example, if you are writing code which is vulnerable to SQL injections and you have already received the feedback but your future MR/PR still has that issue then yes, it will be performance issue.

8
  • 9
    As henning commented on the post, this is a great way to subvert review comments and turn them into a political tool. Now every comment on a review can either be used to pull down a competitor or to enhance a friend.
    – DaveG
    Jul 6 at 13:11
  • 2
    @DaveG I don't see how? If your code doesn't have any security related issue which can be consider as dangerous then i don't see how it can be used as political tool. As said, our Manager who evaluates this is technical person.
    – DS9
    Jul 6 at 13:28
  • 5
    If a reviewer is competing with or doesn't like someone, they will flag everything they can. If they do like the person, they'll suggest changes verbally or in email.
    – DaveG
    Jul 6 at 14:06
  • 5
    @DS9 If I think you're a good contributor to the team and a solid 8/10 but I see an area where you could improve, telling you so harms you rather than helping you. Maybe I think your method naming could stand to be improved, but it's not so bad I want you denied a raise because of it, so I'll keep my mouth shut.
    – mjt
    Jul 6 at 14:31
  • 1
    @DaveG If you have a formal change control process, then everything gets flagged, always. If you don't have a formal change control process, then it's fundamentally impossible to use review/change comments for tracking performance. And if change requests can be passed verbally or in emails without being formally logged, then you don't have a change control process.
    – Graham
    Jul 6 at 14:41
1

So first off, as a business owner and former developer myself, I can say an in-time commit message is a wonderful opportunity to see feedback from a senior developer to a junior developer complete with all the context of why someone feels the way they feel about your performance. This is valuable information for your manager to have and to inform their decision on how to grade your performance.

Unfortunately, they have squandered this opportunity by not reviewing the entire context and instead drawing a conclusion they probably should not have. The entire point of peer review (presumably, why the other developers were commenting in the first place) is to improve that code and the person writing it. I've seen 10x developers (you know, the folks that only seem to stop producing incredible quality code for brief moments when chumps like us interrupt them) get called out in a code review for things they 100% should have caught. These are folks who literally are as productive as the entire rest of the team, but I'm guessing your manager would have told them they need improvement too.

There are a number of things at play - last year was a rough year on everyone, including budgets. They may only have so much available for budgets this year, and since bonuses and raises are tied to performance, they need a reason to not give you as big of one.

The best thing you can do is to take it in stride. Ask for specifics on what improvements they want to see from you. Honestly, you should do that no matter WHAT their grade is. You could get an A+ or whatever you have and you should still ask that question.

Be aware that this may be a sign of a financial issue with the company. I hate to end answers with "brush up the resume" and I'm not saying you need to, but I am saying be aware of other potential red flags. This is a light pink flag - look for others, and if you see too many indicators of trouble, go find another job while you're still employed. (It's a lot easier that way.)

0

Yes & no. Not normal for them to explicitly tell you but totally normal for people to lose their job over it. So then I guess it's not totally insane for you to get it mentioned to you in a performance review. If you keep getting the exact same suggestions to improve your code over and over again then there's a problem like: a.) you're unable or uninterested in learning, b.) you're just difficult to work with (on this particular job).

In my career, the only time I saw that it became a huge issue was with a guy who was still in his probation period. He got terminated because he never listened to anybody. He had 120ish repetitive comments on his work (something most people would do with 0-6 comments) over a few months and never got anything approved because he never listened and thought he knew everything. That just doesn't fly. Usually, it's not an issue because people become experienced enough to do things the right/accepted way very quickly.

The fact they waited till your review to say something is strange to me. From what I've seen: If you're experienced (and proven on the job) and let's say you did something very different from the normal way (accepted at the company), people probably wouldn't even do any negative PR comments. Lead/boss would just have a talk with you and give you the chance to correct things without even making a big deal about it in review comments. You just wouldn't get your code approved till it was "right".

Anyhow, I guess, decide if you want to be "right" or thought of as right. Ha. Pick your battles as they say.

0

If the question is "Is it normal to be done for everyone and without any advance warning," then no, I don't think it's normal, and I also don't think it's fair. This manager would have sprung arbitrary new standards of judgement onto people, probably trying to be 'clever' while not really understanding the purpose of pull request comments.

It'd be like saying:

"Starting from today we're going to judge you based on how many days a week you come into the office. Oh, looks like you only come in three days per week, while [other employee] comes in every day, so [other employee] gets a promotion and you need to go on a performance improvement plan."

That being said, it sounds like what might have actually happened is that people in your team raised an issue, and they went back through your version control history looking for evidence. If that's the case, then it does seem reasonable to use pull request comments as evidence in a larger case, but not necessarily in isolation.

-1

I'm not commenting on your code prowess, your teammates, or what the review process is like in your team. Just the method and implementation for your performance review.

The goal of a performance review is to have clear and actionable methodology that both manager and managed person agreed upon. Normally such methodology is explained by the HR dept. at the start of the process, clear performance indicators are detailed in the process, goals are set/negotiated, indications of achievement is specified in the process, terms of process start/end are set, results are analysed and conclusions are drawn. None of that happened here IMO.

Unless your entire team was made aware that the review comments were an indication of performance, and the whole process of reviewing review comments and assigning performance levels based on their contents was discussed beforehand, his evaluation is meaningless.

Your manager could have divined your performance level in chicken guts with the same results.

I would ask for a 1 on 1 talk with him, explain that he can't just make up a metric and evaluate your performance on it and expect you to agree with the evaluation post factum and without any input from you. If his evaluation results in any loss for you (emotional included), I would raise the issue with HR, telling them you were subjected to an unfair and biased evaluation process where you had no input. If they deem your complaint baseless or ignore it, just find another project/job. Your manager is incompetent and the HR dept. just ignored your plight.

7
  • 1
    You think complaining to HR will help? Is HR more qualified to read PR comments than the manager? HR is gonna review this, then send an email to the manager outlining the steps needed to terminate an insubordinate employee if the manager wants to do down that route. Honestly, for a completely non-technical manager, using PR comments as a basis really isn't that crazy. If the same comment keeps coming up in PRs, certainly a manager isn't wrong to dig in and wonder why it keep happening.
    – Graham
    Jul 7 at 20:54
  • Yes, a formal complaint to HR will help with a potential lawsuit for unfair treatment by the company and its representatives.
    – BoboDarph
    Jul 8 at 7:35
  • Telling your boss that he just can't develop a way of assessing your job performance and then measuring you against that system doesn't sound like a master move. It's his job to assess employee performance. And yes, he can draft the assessment without your input, in fact most assessments have traditionally been done without employee input, as most employees will report that they are doing an incredible job with no room for improvement if there's a chance of getting a little more money, regardless of reality.
    – Edwin Buck
    Jul 8 at 7:36
  • @BoboDarph There is difference between unfair treatment and unfavorable treatment. If this manager reviews everyone's commit / review logs, it's fair (even if some people's comments are not).
    – Edwin Buck
    Jul 8 at 7:37
  • We are not discussing unfavourable treatment here. We are talking about unfair treatment as covered by most laws even for at-will employees in the US. For which the first step do demonstrate goodwill is to create a HR complaint. I am not your legal counsel and usually such things are paid, but if you want to educate yourself, you can always google.
    – BoboDarph
    Jul 8 at 15:20
-3

my team leader, who is a non-technical person

That's your problem, right there.

It is not a good idea to appoint a non-technical person to manage a team of developers (or any team that includes developers). A manager must also be an agreeable and positive, constructive, people-person. The manager should have the attitude that they are serving you, not the other way around.

It might be appropriate for a team leader to review PRs and ask you respectfully about the comments, but it is not appropriate to use this as the basis of a performance review without even talking with you about it. A performance review typically goes on your record and might harm your career prospects.

I suggest to find a better place to work. Freelancing is good.

If you don't want to leave, you could ask the company leaders to find a better manager for your team, but that is unlikely to happen. High-level leaders are very often arrogant as a result of their success and their position of power. They are often unable to admit that they have made a mistake.

4
  • 3
    By this logic, a CEO of a company would also have to technically skilled in every discipline of every employee underneath them. If you object to that, then my question would be: at what point of the hierarchy does the manager not need to be technical? If you arbitrarily pick a spot in the org chart where that shift happens, then you just need to accept that at some companies, that shift happens between devs and their bosses. Is it ideal? Maybe not, but how else is a company supposed to be ran? Everyone can't work for 10-person startups.
    – Graham
    Jul 8 at 12:31
  • We delegate things to others because they might be better at it than us. That also goes for dev teams - while it is nice to have a technical team lead, that isn't necessarily mandatory.
    – T. Sar
    Jul 8 at 19:40
  • 1
    As a coder and a manager and a mentor, I have to say this is terrible advice.
    – Tony Ennis
    Jul 8 at 22:07
  • @Graham, the manager and leader of a group of developers had better be technical. The manager of that manager need not be technical. The CEO need not be technical, unless it is a tech-focused company. I haven't done science or research on this topic, my opinion is simply based on my own reasoning and my own experience programming for many companies and clients over 20 years. There is a world of difference working under a tech leader (need not be the strongest), compared to a non-tech leader who has no clue what the developers are doing. None of you gave a substantial counter-argument. Jul 24 at 20:37

You must log in to answer this question.

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