2

I work as a full stack developer in a team of 7 developers. I like to receive the constructive criticism as well as give it when needed.

I love to review PRs and strive to maintain high coding standards and beautiful code.

However, not all people like criticism, and some take it in egoistic manner. I have a colleague on my team who thinks it's necessary to argue with every comment I make on his PR. For example, if I put a 3 line comment explaining how the directory structure should be organized in certain manner, he will reply almost instantly with a 9 line comment with Wikipedia definitions and what-not to prove that what he did is right. He simply resists every single comment every single time.

One thing he didn't agree on is declaring some variables globally. He preferred to create/repeat the same variable with same query in 7 different functions in a Class file. This is just one example. There are plenty more example where he thinks he is right but is plain wrong.

He makes me feel I am fighting in PR comments with him, and I hate that the most, since the full team has access to the comments. I have reached the point where I have decided not to make any comments on his PRs as it seems pointless.

Unfortunately, other colleagues don't give a damn about what gets pushed to production and have an "as long as it works" attitude. However, they take my criticism positively if I comment on their PRs and don't argue unnecessarily.

How should I handle this situation?

Edit: seems like my global and local variable example blew out of prpoportion in comment sections. Obviously, I did a bad job explaining that in this question. What I wanted to say is if there are multiple helper functions which do exact same query. Isn't it a good idea to put it into a constructor or make it global instead of doing same query multiple times?

10
  • 18
    globals are evil. Having variable in each class is better. That you choose this as an example is worrying. Did you consider that he may be right, and possibly the only one in the team to see the shortcoming of your approach (or the only one to care) ?
    – Jeffrey
    Jul 8 at 3:59
  • 9
    So he writes code. You tell him why you think it is wrong. He defends the code he wrote. And this is a problem for you. Why do you think he is giving you resistance, and not vice versa? Why is your directory structure "right" and his "wrong"? As said above, reusing globals is mostly a bad idea also (think about typos). As far as I can see, the title of your question should be "How do you handle colleagues who hvae opinions of their own?" Jul 8 at 6:05
  • 9
    One final thought, we have a rule in our company (and I expect most other companies are similar), unless code is obviously buggy, or unreadable, or violates aligned standards, we allow the original developer the final say. This is to avoid code reviews getting bogged down in pointless differences of opinion on style. You have shared your point of view, and that is fine, but otherwise you should let it go. Jul 8 at 6:19
  • 2
    Just noting that both using global variables for things where local is enough and using non-standard directory structures are bad ideas. Don't push them into production code, unless you're paying for the fun and don't expect others to accept them.
    – ojs
    Jul 8 at 7:00
  • 4
    @creamCheeseCoder How often have you responded to one of his defenses with "You know what, you're right"?
    – Kevin
    Jul 8 at 13:33
4

It sounds like there's a lack of trust present in this team. To be honest, to me it sounds like the colleagues who 'don't resist' might be doing so out of convenience since, as you say, they don't seem to care about quality and will just take the easiest path. In that regard, the colleague who 'resists' might just be disagreeing with you, and the two of you may have something in common in that you do care about quality and are willing to disagree with each other.

'Constructive criticism' is a two-way process; the one doing the criticising needs to be able to listen to the one they're criticising to find out why they did things that way. I suggest reading Stephen Covey's 7 Habits, particularly habit 5: Seek first to understand, then to be understood.

Fixing a lack of trust can't be done overnight, but here are some tips from what I've found has worked for teams I've been in.

If some feedback is based on your opinion, make it clear that this is the case. You might not like that he wrote his code a certain way, but is there anything objectively wrong with it? At best, you can suggest that you wouldn't have done it that way and provide some reasoning, but it doesn't necessarily mean that one of you is right and the other is wrong.

If something looks odd to you, ask why it was done. There may be a valid reason, and it may just need a renamed variable or code comment to explain it better.

Ask what they think. If you see something that you'd change, instead of dictating how you think it should be done, say what you would have done and why, and ask whether they agree.

Include positive feedback as well as negative. If all that happens when your colleague pushes code is that it gets criticised, then it's not going to be something he enjoys doing, and he'll go into it with a defensive mindset. Can you point out instances where he's done something well? Simply acknowledging a good solution can go a long way towards building that atmosphere of trust, letting him know that you're not just out to tear his work apart, but to encourage and support him.

Don't (inadvertently or otherwise) make review comments into personal attacks. Things like the tone and choice of words can determine how a piece of text comes across. For example, compare:

"This doesn't make sense."

(i.e. this is objectively wrong and you're stupid)

With:

"This isn't how I expected this to work. Can you please give some brief reasoning?"

(i.e. I don't necessarily understand or agree with your solution, but I'm not saying you're wrong, and I'm willing to listen to your explanation)


I can tell you that after working like this for a while, teams really open up and code reviews don't become dreaded; they become a good way to get feedback, and for others to catch mistakes you may not have seen. When someone points out to me that there's a better way of doing something, I actually feel excited to implement it because I know it'll be an improvement to the overall code quality, and because I know that the person giving the feedback isn't out to personally attack me. I hope you can change the perspective within the team from being 'right' and 'wrong' to having valid (and valuable) differences in opinion and style.

1
  • This answer is really insightful and helpful. Accepting it. Jul 10 at 6:23
2

First, be careful with the phrase "maintain high coding standards and beautiful code.".

Regarding Standards

Who decides what the standards are? There are many code-style standards out there, and most are equally valid, as long as a single one is used consistently within a codebase. If you are the product owner, manager, or the undisputed most senior developer, then maybe you do have the discretion of picking the standards, but you have to pre-establish them rather than start making decisions on PRs. The documentation or readme files of the projects should make clear what the standards being applied are. In general, it is not a rule if it is not written.

Regarding 'beautiful code'

While I myself enjoy beautiful code, I see a red flag on any reviewer who justifies his comments with remarks like "this looks ugly", or "I'd be happier if you did it some other way". If code is messy and harder than necessary to understand, that can be a valid point when you can provide elegant alternatives.

But remember that everyone likes to have some degree of autonomy in their work. If every code you review needs to be done "your way", then you might have started to (unconsciously) believe that the code is meant to please you rather than to achieve some company goal and deliver some customer feature. So unless you own the company, keep in mind that "pleasing you" is not the goal of any code. Furthermore, if your reviews basically throw in the garbage the work done by other people, they will not enjoy doing it in the first place, they rather say "then do it yourself next time" (and I do note here that the goal of the code is also not to entertain the coder). In general, up to reasonable limits, and as long as requirements are met, the preference on deciding some implementation details should belong to the person writing and maintaining the code, rather than to the person reviewing (some exceptions on this to very experienced reviewers, who should have management or clear leading roles or are responsible for cohesion in a large codebase).

All that being said, and just to be clear, you do not seem like an unreasonable reviewer, but parts of your questions can raise red flags if extrapolated a lot (which I'm doing for the purpose of this answer).

Regarding the Wikipedia example

Avoid 'correcting' what is not wrong. It is as simple as that. If it meets requirements, if it does not breach any written/established standard for the codebase and if it does not introduce any bug or security flaw, then it is not wrong. If it degrades performance, then you should really have clear performance metrics to show and prove your point.

You can, and maybe should give suggestions, but as others alluded, you should mark them clearly as suggestions or as opinions so that the author of the PR knows that he/she has the option to simply ignore your remark. I worked on a project where code reviewers had the obligation to assign a "relevance" rating to every comment they made. The ratings were either "critical" for serious stuff that should be corrected right away, "regular" for usual stuff that needs correction but has very low impact, "minor" for stuff like naming conventions that had no functionality or performance impact whatsoever, but had a mandatory correction and finally "Information" for stuff that was just an idea for an alternative solution or an opinion.

While people generally accepted those "suggestions", the fact that they were clearly stated as optional prevented a lot of pain that would ensue on pointless discussions such as "this could be marginally better but would also mean a lot of rework", "If you'd like to adopt this standard here, then there are 134 files which would also need to be updated", "I have a different opinion and I'm the one doing the code here" and so on.

Regarding the global variables example

From what you've written I cannot form my own opinion on who's wrong or right (if anyone) in that case, so I'll assume three cases:

  1. IF You are right: Then you may need to communicate better and have a well-established point. You need to teach the person a lesson, not in the "vengeance" sense but in the "classroom" sense. Instead of fighting over PRs, sit with him with a computer nearby and make a presentation showing objectively why he is wrong. Preferably write code that proves he's wrong and run it in front of him. For example, if you need to explain that you need a specific copy operation to copy a list on Python, then write and run an example for it as as in here, don't simply tell him that "what he did is just plain wrong".

  2. IF you are wrong: You may find out this is the case during the exercise of preparing the aforementioned presentation. This is great because you've done some understanding before criticizing. Always keep in mind that everybody (including you) makes mistakes. And there are different levels of tolerance towards different types of mistakes. If you are going to publicly claim that someone is wrong (you've mentioned public PRs), then you need to be very much sure that you are right. In case you aren't, a personal talk where you ask the person to explain what he's done is maybe more than enough for him/her to enlighten you (or in the previous case, they might notice their mistake themselves while attempting to explain).

  3. IF neither of you is wrong or right: Then you are discussing matters of opinion. Ask yourself: Why are you discussing a matter of opinion? You apparently don't enjoy exchanging ideas with this particular colleague, since you expect him/her to simply comply with your suggestions rather than try to learn something from what he/she answers to you. Why does it matter if it was made in the way he/she prefers rather than the way you prefer? Does it actually have an impact on other pieces of the codebase? Then point this out, so you share the relevant information with the whole team to call for a decision that affects more than the single PR written by your colleague. Otherwise, remember as before not to correct what is not wrong.

Regarding public PR discussions

There is an old adage that says "Praise in public, criticize in private". Public comments on a PR are supposed to be documentation, not discussion let alone criticism. Mistakes happen, and not writing anything to any PR will make it look like you are not doing any review work at all, hence why PRs reviews are (sometimes) public. But if you feel bad about discussing in public, so does the other person, and there is no surprise that he/she gets defensive. You are criticizing in public. My suggestion is that if anything gets messy in the review you should have written notes of your own, and talk in private with the person, either through a video call with a shared screen or a meeting room, then go over your notes, explain your points and reach agreements on what should be done on the code. Then, these agreements can be posted as the PR review, but preferably write them with the other person present and agreeing on the content. This avoids getting the person too defensive and separates the fighting points for private while the relevant/straightforward remarks can be put on the PR review. This may be time-consuming, but if you took note of the previous points, this should all be worth the time it takes.

Regarding the "as long as it works" attitude

Yeah, I get this part is difficult. If you are in a management position, you have a lot more power to change this mindset. If you aren't, then you need help from management. A few suggestions that may help:

1. Files have owners. Create a rule that each file in the codebase has an owner. If changes need to be done in a certain file, the owner will do it. If the code broke in that file, the owner will fix it. Someone other than the owner is responsible for reviewing it each time, the reviewer should change periodically. That does not mean the owner has the last word on how the code works in that file, but it at least makes clear that poorly written stuff will come back at the person who wrote it. Reviews remain important so that no one produces hieroglyphs to make him/herself unfireable.

2. Good code means replaceable people. Managers hate having unreplaceable people. If code is too poorly written to the point only the author (if anyone) understands it, then this becomes a liability to the code. I've heard about cases of entire codebases basically dying because it was not worth the effort to maintain it. And this favors the poor performers rather than the people looking up for promotions. That is mostly an argument to get management on board with improving code quality.

3. Consider having a separate QA department. In some companies (think mechanical engineering rather than coding), there is a quality assurance team whose job is to verify compliance with requirements (both functional/performance but also quality and standard adhesion requirements). These are normally guys that do not answer to a manager, but rather directly to a director of operations, so that they cannot be silenced or retaliated by a manager pressed to meet a deadline. They don't come up with the rules, they just enforce them and check if those are being followed. Maybe a squad like this should be performing reviews rather than peers.

4. Let it be. As mother mary said, when finding yourself in time of trouble, let it be. You might have thought that the previous suggestion would be way too overwhelming and exaggerated for your company. But here's the news: Maybe your code standards or preferences are also too much for your company. In many businesses, time to deliver features is much more critical than code quality. Perhaps because if they grow enough the entire codebase may need to be fully reworked on a more scalable solution, perhaps because some performance issue has very little impact on profit and customer experience but additional features would open new markets and make the product compete against the super-hot expensive alternative that dominates the segment. Also, the company may be having a hard time hiring developers, so keeping the ones they have happy without increasing salaries too much is becoming a relevant concern. All those are reasons why many startups have surprisingly low coding standards, that will only improve once the company gets acquired and the new shareholders put some adults in place to enforce sustainable and rigid coding practices. And even then, some colleagues of yours will prefer to quit the company for another one in which coding is a much less strict activity.

1

Figure out their motivations and reasons and see if this is compatible with them.

Fundamentally, the priority of many employees is keeping the boss happy, especially for today. That is their first and foremost goal and it is almost certain to be high up for the rest. It is certainly high up on priorities.

If you are at a company where people are evaluated primarily on how many features you produce, there is no reason for an individual to bother to fix code quality issues as the reward is for shipping features. This is especially true if there is someone nagging you about the feature who wants it in prod.

The fact that such behaviour is the norm tells me that it is a very results oriented workplace, to the point that development is treated as a black box and as long as the developers do their magic, everyone is satisfied.

There often is not much you can do, as you are fighting the prime directive of keeping management happy. I have been that developer pushing bad code, as someone wanted it fast and pushing back was a pointless hassle in my view. I have approved bad code after another developer tried to stop that same code from going to prod and the dev asked me to give it a thumbs up instead. Given what management wanted and prioritized, you couldn't have altered my behaviour (you might alter my apparent behaviour, but that would just mean I would collude with the other developers to go around you).

That developer who tried to prevent bad code in production was viewed as slow and unproductive by management.

TL;DR: Code quality is clearly not important to management here, so you probably cannot fix it as then the developers are not going to care.

2
  • I agree with you partially. Keeping boss happy is one thing and generating tech debt is another. If bad code constantly gets pushed to prod then in future we will be doing unncessary optimizations while the core features staying in backlog for months. Jul 8 at 3:22
  • @creamCheeseCoder 1. how long do people intend to stay? It may not be their problem at all. 2. Everyone being slow is fine. The problem with keeping management happy is if I am slow compared to other people. Jul 8 at 3:32
0

I will answer the general question "colleagues who constantly give you resistance?" and not comment the precise examples given. I have had the same situation in the past so I will try to explain what worked for me. My advices are:

  • Do not take it personally. Your colleague has probably the same behavior with other people.
  • Do not stop writing remarks if something is definitely wrong: one aspect of PRs is that you can look back for information later on. If problems occurs in the future and you had already pointed them out during the review phase, then it won't be your problem. If your colleague refused to correct it, that will be his issue.
  • Do not think you can change him: this is really the personality of certain people.
  • If you get to chose your projects/tasks: try to avoid him. Discussions and arguments are not a bad thing as such if it allows moving forward. However some people tend to try to impose their opinion all the time: people don't enjoy working with them anyway and will avoid having to collaborate with them.

Majority of people are not like that, do not lose hope :).

2
  • 1
    I think you got it the wrong way: The OP is definitely wrong with his remarks and they should keep them to themselves.
    – ojs
    Jul 8 at 13:13
  • Correct indeed. I've adapted my comment to be more generic as I did not want to specifically react on the example.
    – han
    Jul 8 at 14:15
0

Here's an idea to help work on the relationship you have with this colleague.

Sit down together away from the day-to-day (cafe, outdoor bench / park), take two post-it notes and two sharpie markers. Given them a sharpie and post-it, you use the other set. Say "I wanted to check-in on how you and I are working. I would like us both to write down a number out of ten on how well we each think our working relationship is performing". A "10" is "it couldn't be better, world class, we should write a book", a "5" is "we do just enough to get by", and a "1" is "the only thing we agree on is the logo on our pay slip".

Have them write their number, as you write yours. Count 3-2-1 and then reveal them to each other simultaneously. This moment will be quite captivating, and is often quite funny - the laugh will do you both good. Then, discuss why you each used the numbers you used, and what stopped you using "10". It will be important for you to own your share of the mess to show that you recognise that your working relationship is co-created and both of you are playing a role in what is happening between you.

You must log in to answer this question.

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