4 ms·

> Code reviews are probably the first thing a new engineer can start doing in a new role. That really depends on what you want the code review to achieve. Cat

> Code reviews are probably the first thing a new engineer can start doing in a new role. That really depends on what you want the code review to achieve. Catch typos? Yes, a new engineer can do that. Check if code fits into the existing architecture, adheres to the invariants of the code base, uses base libraries idiomatically etc? I don't think a new engineer can contribute that from the start. > Creating metrics through issue trackers and time sheets Which metrics? It's far too easy to create metrics that are easy to measure, rather than metrics which actually increase the business when optimized for (and developers will optimize for / game a metric when it's used to assess their performance).

ashishuthama 7y ago
A new engineer can learn a lot from being on code reviews earlyon. Anything from the team coding style to the application architecture.
- jolmg 7y ago
  A reviewer is not there to learn. If anything, they're there to teach. What good is a reviewer that can't grade others on their adherence to the coding style and application architecture of the project? They're supposed to determine what goes in or not.
  - wolco 7y ago
    They wouldn't be the only one reviewing or reviewing at all. It would help them understand the process if they see it in action.
  - stevens32 7y ago
    Reviewers not willing to learn anything during a code review is probably an easy tell for a crappy code review culture
    - jolmg 7y ago
      This discussion is about newcomers that know basically nothing of the project. When I say they're not there to learn, I mean their primary function as reviewers isn't to learn from what gets submitted. They can learn depending on what gets submitted, but if 90% of the time their ignorance doesn't permit them to properly determine if what's submitted to them is good or not, then what's the point of their review?
vinceguidry 7y ago
Maybe have a senior review the code afterward so the new guy can see what's being expected?
- jolmg 7y ago
  So the new guy is just watching the reviewer? That sounds neither productive, nor an efficient way to get familiar with the project.
  - rleigh 7y ago
    That's not what they said. You learn by doing, and also by example. So they can do the review themselves, but also see what other people pick up on. It sets expectations, and also shows how other people think. I always find it interesting when I do the same review as one of my co-workers. We might pick up on the same things, but often they are completely different. It shows the value of having multiple perspectives on a team. Not just for review, but for design, implementation, validation, debugging, and the rest. It widens people's perspectives, serves as a means for ongoing improvement and learning, and makes the team better as a whole. We all have different backgrounds and expertise, and having people share and pass on some of their knowledge and skills is highly valuable.
shados 7y ago
Code reviews have 2 purpose. Help the person whom's code is being reviewed improve quality/catch issues, BUT ALSO the reviewer gets familiar with the code being pushed and learn stuff. It's super important for new engineers to review code as soon as possible for the later.
- jolmg 7y ago
  > BUT ALSO the reviewer gets familiar with the code being pushed and learn stuff. The reviewer is supposed to determine what goes in or not, so how can this be an opportunity to learn from what goes in? They're the one who's supposed to determine that! Is your idea of a reviewer someone who just spectates all code going through? It's like saying people who don't know a subject should grade work from students taking a class in that subject, as it's a great opportunity to learn from what gets turned in.
  - BeetleB 7y ago
    >The reviewer is supposed to determine what goes in or not This is not a given, and in fact, I would not recommend it. For small code reviews with only one reviewer, the reviewer gives feedback and they have a conversation about it, but with only one reviewer the developer should have the final say. The problem with giving the final say to a single reviewer is you'll get plenty of pointless code changes to suit the reviewer's individual preferences. Don't waste time on what is a perfectly reasonable difference in opinion. If you have more than one reviewer, but still a small code review, the final say should be with the reviewer(s) - if their is consensus (you could also go with voting, with its pros and cons). Having multiple reviewers prevents the likelihood of individual preferences dominating. If it's a significant code review, the final say should be with an experienced moderator.
  - tkxxx7 7y ago
    > The reviewer is supposed to determine what goes in or not, so how can this be an opportunity to learn from what goes in? You specify multiple reviewers. That's it.

ashishuthama 7y ago

A new engineer can learn a lot from being on code reviews earlyon. Anything from the team coding style to the application architecture.

jolmg 7y ago
A reviewer is not there to learn. If anything, they're there to teach. What good is a reviewer that can't grade others on their adherence to the coding style and application architecture of the project? They're supposed to determine what goes in or not.
- wolco 7y ago
  They wouldn't be the only one reviewing or reviewing at all. It would help them understand the process if they see it in action.
- stevens32 7y ago
  Reviewers not willing to learn anything during a code review is probably an easy tell for a crappy code review culture
  - jolmg 7y ago
    This discussion is about newcomers that know basically nothing of the project. When I say they're not there to learn, I mean their primary function as reviewers isn't to learn from what gets submitted. They can learn depending on what gets submitted, but if 90% of the time their ignorance doesn't permit them to properly determine if what's submitted to them is good or not, then what's the point of their review?

vinceguidry 7y ago

Maybe have a senior review the code afterward so the new guy can see what's being expected?

jolmg 7y ago
So the new guy is just watching the reviewer? That sounds neither productive, nor an efficient way to get familiar with the project.
- rleigh 7y ago
  That's not what they said. You learn by doing, and also by example. So they can do the review themselves, but also see what other people pick up on. It sets expectations, and also shows how other people think. I always find it interesting when I do the same review as one of my co-workers. We might pick up on the same things, but often they are completely different. It shows the value of having multiple perspectives on a team. Not just for review, but for design, implementation, validation, debugging, and the rest. It widens people's perspectives, serves as a means for ongoing improvement and learning, and makes the team better as a whole. We all have different backgrounds and expertise, and having people share and pass on some of their knowledge and skills is highly valuable.

shados 7y ago

Code reviews have 2 purpose. Help the person whom's code is being reviewed improve quality/catch issues, BUT ALSO the reviewer gets familiar with the code being pushed and learn stuff. It's super important for new engineers to review code as soon as possible for the later.

jolmg 7y ago
> BUT ALSO the reviewer gets familiar with the code being pushed and learn stuff. The reviewer is supposed to determine what goes in or not, so how can this be an opportunity to learn from what goes in? They're the one who's supposed to determine that! Is your idea of a reviewer someone who just spectates all code going through? It's like saying people who don't know a subject should grade work from students taking a class in that subject, as it's a great opportunity to learn from what gets turned in.
- BeetleB 7y ago
  >The reviewer is supposed to determine what goes in or not This is not a given, and in fact, I would not recommend it. For small code reviews with only one reviewer, the reviewer gives feedback and they have a conversation about it, but with only one reviewer the developer should have the final say. The problem with giving the final say to a single reviewer is you'll get plenty of pointless code changes to suit the reviewer's individual preferences. Don't waste time on what is a perfectly reasonable difference in opinion. If you have more than one reviewer, but still a small code review, the final say should be with the reviewer(s) - if their is consensus (you could also go with voting, with its pros and cons). Having multiple reviewers prevents the likelihood of individual preferences dominating. If it's a significant code review, the final say should be with an experienced moderator.
- tkxxx7 7y ago
  > The reviewer is supposed to determine what goes in or not, so how can this be an opportunity to learn from what goes in? You specify multiple reviewers. That's it.