3 ms·

It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Also, I'd like t

by imiric 4y ago

It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Also, I'd like to push back on this: > Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line. I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of the main tasks of a developer, perhaps more important than writing code. Reviewing code gives you an opportunity to get familiar with other parts of the code base, understand why a change or fix is done, and collaborate with others to make an overall better improvement to the product. It gets frustrating when egos get involved, and people take criticism too personally, or point out improvements for the sake of demonstrating their knowledge. In best working environments, code is judged without a bias or relation to who produced it, but as an inert output of the collective team. I would actually like to see a feature in code review tools to submit merge requests anonymously, just so that the author isn't a prevalent factor while reviewing. :) I usually dedicate a few hours, maybe even half a day, to reviews. Definitely don't look at it as a chore to put off, but as a crucial part of your responsibilities as a developer.

joshka 4y ago
If you read that statement as "when judged on your output by those in control of your salary" does it make more sense which bottom line is in play here?
- imiric 4y ago
  It depends. If your employer is a company that values good software development practices, then they wouldn't judge you based on the LOCs you produce, but on the overall value you provide to the team, product and company. If they're not, then most of what we're discussing here is not very relevant, since they likely don't follow other good practices either. If you're in that situation, then you have a few options: if you value the product and company, promote these best practices so that the company can attract and keep good developers, which ultimately benefits the product. If you don't, or they won't listen, or you don't have the authority to change things, then you can probably forget about following good practices, and just keep working based on the metrics they do value. Or, I'd strongly suggest, just find a better working environment. :)
weego 4y ago
I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of the main tasks of a developer, perhaps more important than writing code. It provides the possibility of not causing future 'your business' reductions to its bottom line. That's why any 'rules' to code review are so context specific and should be applied as such.
gernb 4y ago
I went ~25yrs into my coding career without code review and without tests. I wrote games, several AAA games, play testers found bugs, but otherwise there was no code based tests and no code review, none, zero, zilch. From 1983 to around 2008. Now I use tests and code review and I like them. I like that reviewers catch my bugs. I like that they suggest better solutions I didn't think of. I like that they tell me about functions I could use that make the code similar. I like that they help explain the code. But, I can't entirely dismiss that I and the teams I worked on shipped a ton of products without any code review and that my velocity feels much slower than previously. It's possible that slowness is from the projects being way more complex, the teams being much larger, and that code review is actually a perf multiplier over all for these larger more complex things I'm working. But, waiting for and/or doing reviews certainly "feels" slower than I used to be.
- kafrofrite 4y ago
  > I like that they suggest better solutions I didn't think of. Although I don't write code full-time, when I do this is the part I enjoy more. People reviewing my code and coming up with better solutions on that same problem amazes me.
- nicoburns 4y ago
  For a game that has a single release code review seems like it would be less important. Where I've found it really comes into it's own is keeping a codebase maintainable over several years.
- imiric 4y ago
  Sure, the programming landscape was much different 30 years ago. We also didn't rely on code versioning, CI/CD, testing, and many other practices we consider standard today. The industry has since evolved to make development better and more reliable, at the expense of relative speed or simplicity. In return, we gained the ability to minimize the chances of shipping broken code that is only caught by manual testers, or, worse, actual users. Game development particularly went through radical changes. I don't have much experience here, but I can imagine the stress of needing to ship bug-free code on actual physical media, and the excruciatingly slow dev-QA-release cycle. We're spoiled these days to have automated tests, peer review, and OTA updates, where bugs can be found, fixed, and bugfixes shipped much quicker than if we were not doing this. So I agree that all of these processes add a mental and time overhead, but I wouldn't go back to working without them for anything.
feb 4y ago
> It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Actually, that's not true at all. Code review is not a replacement for automated builds and testing. That's what Continuous Integration is for. Let humans concentrate on what they do best (understanding and comment on code) and don't lose their time and patience with things that can be automated. Projects often enforce that by running CI on code submitted for review and it cannot be accepted as long as a CI fails. It also reduces breakages for other devs of the builds of the code base.
- imiric 4y ago
  > Code review is not a replacement for automated builds and testing. I agree, and never said it was. But the fact is most projects don't have complete test coverage, or even replicate the real world environment of actually using the program. In the absolute best case scenario where they do, I agree with you; automated tests and CI should find any functional issues. But if your software is meant to be used by a human, then at some point it should also be tested by one. Preferably before your actual users test it for you. It depends on the project, of course, so if all you're working on is a library or API consumed by machines, then by all means, rely entirely on CI if you can. Same goes if your project is a small piece of a much larger machinery, and it's difficult to test locally, then it makes sense to rely more on CI in that case as well. But you can't expect humans to actually interpret all the nuance and complexity of a non-trivial code change from just staring at patches. At least, I'm not that smart. So I typically pull the changes locally, go through the relevant files, try to manipulate inputs and see what the effects are (i.e. manual fuzzing), compare the behavior before and after the changes, and, finally, since I already have the code locally, build and run the program as a last quick check that things actually work. Sure, this is time-consuming sometimes, but it's been helpful for finding issues or just understanding the code more than if I wouldn't do this.
  - lamontcg 4y ago
    I expect the submitter to have done that. The most annoyed I get in code reviews when it is clear that the tests are green but the code is totally broken and the submitter has never bothered to test it. And at my old job there were things that to properly do a manual QA would have taken all day because it wasn't possible to test on a laptop (e.g. wrong O/S and no ability to easily spin up an AIX image to start with). Really feel like this needs to not be a job of the reviewer, but needs to be on the submitter, but a lot of junior devs or open source contributors tend to skip that step and treat green tests as the definition of done, rather than working software.

by imiric 4y ago

joshka 4y ago

If you read that statement as "when judged on your output by those in control of your salary" does it make more sense which bottom line is in play here?

imiric 4y ago
It depends. If your employer is a company that values good software development practices, then they wouldn't judge you based on the LOCs you produce, but on the overall value you provide to the team, product and company. If they're not, then most of what we're discussing here is not very relevant, since they likely don't follow other good practices either. If you're in that situation, then you have a few options: if you value the product and company, promote these best practices so that the company can attract and keep good developers, which ultimately benefits the product. If you don't, or they won't listen, or you don't have the authority to change things, then you can probably forget about following good practices, and just keep working based on the metrics they do value. Or, I'd strongly suggest, just find a better working environment. :)

weego 4y ago

I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of the main tasks of a developer, perhaps more important than writing code. It provides the possibility of not causing future 'your business' reductions to its bottom line. That's why any 'rules' to code review are so context specific and should be applied as such.

gernb 4y ago

I went ~25yrs into my coding career without code review and without tests. I wrote games, several AAA games, play testers found bugs, but otherwise there was no code based tests and no code review, none, zero, zilch. From 1983 to around 2008. Now I use tests and code review and I like them. I like that reviewers catch my bugs. I like that they suggest better solutions I didn't think of. I like that they tell me about functions I could use that make the code similar. I like that they help explain the code. But, I can't entirely dismiss that I and the teams I worked on shipped a ton of products without any code review and that my velocity feels much slower than previously. It's possible that slowness is from the projects being way more complex, the teams being much larger, and that code review is actually a perf multiplier over all for these larger more complex things I'm working. But, waiting for and/or doing reviews certainly "feels" slower than I used to be.

kafrofrite 4y ago
> I like that they suggest better solutions I didn't think of. Although I don't write code full-time, when I do this is the part I enjoy more. People reviewing my code and coming up with better solutions on that same problem amazes me.
nicoburns 4y ago
For a game that has a single release code review seems like it would be less important. Where I've found it really comes into it's own is keeping a codebase maintainable over several years.
imiric 4y ago
Sure, the programming landscape was much different 30 years ago. We also didn't rely on code versioning, CI/CD, testing, and many other practices we consider standard today. The industry has since evolved to make development better and more reliable, at the expense of relative speed or simplicity. In return, we gained the ability to minimize the chances of shipping broken code that is only caught by manual testers, or, worse, actual users. Game development particularly went through radical changes. I don't have much experience here, but I can imagine the stress of needing to ship bug-free code on actual physical media, and the excruciatingly slow dev-QA-release cycle. We're spoiled these days to have automated tests, peer review, and OTA updates, where bugs can be found, fixed, and bugfixes shipped much quicker than if we were not doing this. So I agree that all of these processes add a mental and time overhead, but I wouldn't go back to working without them for anything.

feb 4y ago

> It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Actually, that's not true at all. Code review is not a replacement for automated builds and testing. That's what Continuous Integration is for. Let humans concentrate on what they do best (understanding and comment on code) and don't lose their time and patience with things that can be automated. Projects often enforce that by running CI on code submitted for review and it cannot be accepted as long as a CI fails. It also reduces breakages for other devs of the builds of the code base.

imiric 4y ago
> Code review is not a replacement for automated builds and testing. I agree, and never said it was. But the fact is most projects don't have complete test coverage, or even replicate the real world environment of actually using the program. In the absolute best case scenario where they do, I agree with you; automated tests and CI should find any functional issues. But if your software is meant to be used by a human, then at some point it should also be tested by one. Preferably before your actual users test it for you. It depends on the project, of course, so if all you're working on is a library or API consumed by machines, then by all means, rely entirely on CI if you can. Same goes if your project is a small piece of a much larger machinery, and it's difficult to test locally, then it makes sense to rely more on CI in that case as well. But you can't expect humans to actually interpret all the nuance and complexity of a non-trivial code change from just staring at patches. At least, I'm not that smart. So I typically pull the changes locally, go through the relevant files, try to manipulate inputs and see what the effects are (i.e. manual fuzzing), compare the behavior before and after the changes, and, finally, since I already have the code locally, build and run the program as a last quick check that things actually work. Sure, this is time-consuming sometimes, but it's been helpful for finding issues or just understanding the code more than if I wouldn't do this.
- lamontcg 4y ago
  I expect the submitter to have done that. The most annoyed I get in code reviews when it is clear that the tests are green but the code is totally broken and the submitter has never bothered to test it. And at my old job there were things that to properly do a manual QA would have taken all day because it wasn't possible to test on a laptop (e.g. wrong O/S and no ability to easily spin up an AIX image to start with). Really feel like this needs to not be a job of the reviewer, but needs to be on the submitter, but a lot of junior devs or open source contributors tend to skip that step and treat green tests as the definition of done, rather than working software.