3 ms·

I see. I want to try and understand this because I am also trying to get better at code reviews and not come across as a dogmatic person. I have spent almost 1

I see. I want to try and understand this because I am also trying to get better at code reviews and not come across as a dogmatic person. I have spent almost 15 years in mostly AWS and I want to keep myself in check and make sure people don't take my suggestions as the vague "quality" as you so mention just because of my seniority. Here is the most recent PR I did for a relatively young person in my org. Part of the PR was doing a type of snake_case to CamelCase conversion from some user inputs to some enums classes. It was done manually with String manipulation and playing with indexes etc. It also came with a bunch of unit tests to make sure the conversion was working correctly. I put in a comment along the lines of "hey, this conversion seems like it can be done with this Apache Commons Text library; given that we are already using it in the same project in some other place, we can remove this manual code and call the library function; we can even remove the tests assuming the well-tested library is doing the right thing". Was this comment warranted? If you think this is a reasonable comment, what computer science principle would you say it is based on?

withinboredom 3y ago
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

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.