3 ms·

There are three parts to every code review: 1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate tha

There are three parts to every code review: 1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate that). 2. Working code: does the PR have a description/ticket and does the code do what it promises to do? Can we refactor anything to make it better? 3. Conventions: does the PR have tests when necessary, are there negative and positive tests? Does it pass those tests? Is there anything against the conventions? If so, bring it up with the dev and find out why, outside of the PR. Maybe they didn't know the convention or there is some legitimate reason for it. Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone. In this particular case: Suggesting a library should have been done before the code was even written. At this point, I wouldn't even suggest using it without some homework for the reviewer and look at the implementation in the library. If the library implementation covers cases not found in the code I was reviewing, I would suggest looking at that library for some inspiration and/or switching to it. I would emphasize switching to it if it is far and away from the library code; as time could be better spent elsewhere. If the new implementation looks better than the library one, I might even suggest refactoring the entire codebase to use the new implementation instead of the library and/or spending some time to open a PR with the original library and improve it. The work has already been done. Try to capitalize on it, instead of dismissing it. Seriously though, look at the library implementation before moving forward. I've seen some very popular libraries and frameworks be very poorly implemented in some areas but because "it's popular" people seem to think it is better. That's not always the case.

redditor98654 3y ago
> Suggesting a library should have been done before the code was even written. Unless you agree on every detail before implementing anything, I don't see how this is practical. At least not in the companies I have worked in. > The work has already been done. Try to capitalize on it, instead of dismissing it. Not quite though. The developer may have written down some code but the work is not done. It needs to be shipped to production, monitored and supported by the oncall etc. If there is a way to not write some code and use a library, I will suggest that. > Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone. If I see a PR with some key tests missing, I don't see why asking if we can add some more tests would be seen as calling them out on it. The PR is a place to record such things - may be it does not need such tests or may be it is intentionally not handled in the code etc. Why would such a discussion be seen as someone being an ass? Why do I have to have that perfectly normal discussion in secret away from the rest of the team? We can ask questions and still be professional. Code reviews are not just for making sure code is good, but also for education - it is a good way for the other to know what is going on and also learn. My read on this is that may be this is what happened to you - someone was being personal and attacked team members personally in the guise of a review and now you take a stance that either discuss everything before implementation in person or have separate meetings in private to suggest changes or ask questions. I am thankful I did not have such a colleague/mentor when I started and hopefully I am not inflicting such an attitude to the new folks that are coming in now.
Arainach 3y ago
>The work has already been done. Try to capitalize on it, instead of dismissing it. Except it hasn't at all. This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it. Successful projects don't just take any code that compiles and passes the tests, they are built in a thoughtful manner with high standards.
- withinboredom 3y ago
  > This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. To be honest, I’m not sure how you got there from here… > The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it. I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart. So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.
  - Arainach 3y ago
    >To be honest, I’m not sure how you got there from here… "I've done all this work, get put of my way and submit" is the attitude in both cases. Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with. The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.

redditor98654 3y ago

> Suggesting a library should have been done before the code was even written. Unless you agree on every detail before implementing anything, I don't see how this is practical. At least not in the companies I have worked in. > The work has already been done. Try to capitalize on it, instead of dismissing it. Not quite though. The developer may have written down some code but the work is not done. It needs to be shipped to production, monitored and supported by the oncall etc. If there is a way to not write some code and use a library, I will suggest that. > Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone. If I see a PR with some key tests missing, I don't see why asking if we can add some more tests would be seen as calling them out on it. The PR is a place to record such things - may be it does not need such tests or may be it is intentionally not handled in the code etc. Why would such a discussion be seen as someone being an ass? Why do I have to have that perfectly normal discussion in secret away from the rest of the team? We can ask questions and still be professional. Code reviews are not just for making sure code is good, but also for education - it is a good way for the other to know what is going on and also learn. My read on this is that may be this is what happened to you - someone was being personal and attacked team members personally in the guise of a review and now you take a stance that either discuss everything before implementation in person or have separate meetings in private to suggest changes or ask questions. I am thankful I did not have such a colleague/mentor when I started and hopefully I am not inflicting such an attitude to the new folks that are coming in now.

Arainach 3y ago

>The work has already been done. Try to capitalize on it, instead of dismissing it. Except it hasn't at all. This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it. Successful projects don't just take any code that compiles and passes the tests, they are built in a thoughtful manner with high standards.

withinboredom 3y ago
> This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. To be honest, I’m not sure how you got there from here… > The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it. I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart. So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.
- Arainach 3y ago
  >To be honest, I’m not sure how you got there from here… "I've done all this work, get put of my way and submit" is the attitude in both cases. Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with. The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.