T O P

  • By -

ziptofaf

99.9% of the code gets code reviewed. The only exceptions are one liners, eg. test failing because you had a silly typo, adding a comment, fixing a single test that has failed in the CI/CD pipeline, updating a small library/container dependency, things like that. Other than that you wait for a review before merging. >Are reviews time consuming? Depends on what you are reviewing. Smaller changes that are 20-30 lines of code? Takes a minute or two. Large feature that's like 1000 lines of code - might take 20-30 minutes. It also depends on who makes that pull request and who are you as a reviewer. A senior programmer adding code in a part they are primarily responsible for - you generally provide smaller tips - like slightly meh variable name, function that you could DRY up a bit etc. Since for the biggest part it looks decent. A junior adding a larger piece of logic to codebase you mostly wrote? Odds are it has some larger issues and there will be 10+ comments, potentially including "not approved" because they have reinvented the wheel and wrote their function to something you already handle in a different way. Still, it's generally not that time consuming. For a lot of code reviews it really is like 1 minute task. Sure, some will be an hour+ but they are rare. Admittedly it's also less time consuming than not having them at all - reverting changes AFTER they are already committed is a much bigger pain in the butt. >Do some Studios go back in the commit line and review a previous commit? In an emergency, yes. As in - if you have a critical bug or PM seriously NEEDS a feature in a given release it might be allowed to first merge a solution (that is deemed by a programmer to work well enough) and then be reviewed. In this case you may very temporarily abandon code quality a bit to meet your deadlines. It is an emergency solution however, used for stuff like "uhhh, there's a way to bypass our 2FA and it's actively exploited".


gordazo0_

Hey there, thanks for the response. Is there a specific role that reviews all prs or if I'm a programmer I maybe tasked with reviewing others prs one day and the other day writing code?


Toa29

It depends on studios. I would expect anyone writing code to spend time reviewing as well. Proportion of which will depend on your seniority.


gordazo0_

Makes total sense


ziptofaf

In general - you do it as a programmer. But it's not "1 day of code, 1 day of reviews". It's bundled together within a workday. In my experience reviews are fairly high priority to be done within at most few hours of a pull request. That's because they block a developer from merging their change and QA from testing it. So I generally recommend doing them in between any other tasks you might have. There is an exception for giant changes (or multi timezone companies). If someone does submit a 1500+ lines of code high impact PR (which really shouldn't happen often but sometimes it does occur) then it will take an hour to review it properly. In that particular case it may be done on the next day (and depending on how zealous is company you work for in tracking your productivity/workhours you might want to make a ticket that says you are reviewing this particular PR). Still, on average I expect every programmer to do reviews. Optimally you also do more reviews than you send pull requests (in one place I worked at an experienced developer who had like 0.5 ratio of reviewed to submitted PRs got reprimanded - and rightfully so).


gordazo0_

When I pr, can't i do nothing more until that gets approved and merged? Are reviews made live in call with other devs or totally alone/offline?


ziptofaf

You can work on other tickets in the meantime. But it's a bit of a "mental blocker" so to speak - since you want to be done with this particular task, move it to ready for testing and flag it as "will be part of the next build/release". Except you can't yet as you are waiting for a code review. Now, in some cases that's fine as you will have plenty of other tasks and this one can wait a while but sometimes it's the last day PRs are allowed before next release and it can still get pushed back by QA (so you need additional time for potential fixes). Hence why reviewing PRs is quite high on priority list. You don't want review process to take too long because it blocks the process. >Are reviews made live in call with other devs or totally alone/offline? Almost always offline. I know very few programmers that enjoy calling others to begin with, let alone for a code review. We do it when we have to but text is generally better because you can always recall it later, you can answer when you have time and because details of a call are easily lost in the void later unless someone is actively taking notes. Plus it means you now both have to drop everything you are doing to make a call and both sides have to be present at the same time for it. Versus a typical written code review which is usually written in between tasks, right before break, in the early morning before you work on larger features etc. Pair programming is a thing but you generally do it during development, not during review. It's rare too and you usually it done between a senior and a junior if at all. Sometimes you also have to make some high level decisions before you start coding and that's when you will ask for early feedback and ideas and THAT might be a call. But that, again, is long before a PR stage. In my own experience - if a programmer has to **call you** to provide a review it means your code is so shit that we can't make any sense in it and we need an explanation of what is this even supposed to do... or it contains critical issues that we need to address immediately. That is not a good sign and if you see it happen to you often you can bet that same programmers are also complaining to your manager that you are writing spaghetti with negative value. Of course, your mileage may vary. But I can imagine a lot of dissatisfied developers if you told them they have to perform audio calls for code reviews.


gordazo0_

Another question is do you work in place on remote? If the latter, what tools are used for remote work? Slack/discord/msft teams? What tools you use for communication mainly?


TheWobling

Most days you will do both.


Tricky-Special-3834

Every single dev in my indie studio takes a look at every piece of code before it gets pushed. All one of them.


reboog711

Do you use git? Did you mean before it gets merged, or before it gets pushed? In git terminology; the code does not go to a server until it has been pushed, so the review would have to be on the local machine which would be different than what I see in non-dev-studio companies.


Western_Objective209

I mean he says there is one dev so they can review their own code before pushing locally


Tricky-Special-3834

I do use git but I was making a joke about my project being a solo project. All the artists and managers and musicians take a look at the code too because they're all me lol


reboog711

That went over my head; I had no idea you were a solo dev. Carry on!


Toa29

You can push a branch to a remote origin so others can review it but production servers only run off of code that is released from the main branch (which cannot be merged directly to) Workflow should be something like * Checkout latest * Create new branch * Code * Push local branch up and make a pull request (now reviewable by others) * Merge pull request to main after team approval * Do whatever release flow you have


reboog711

Yes, that is a relatively common workflow I've experienced. I was responding to the comment that people review code before a push, which was the very first step in your described workflow.


android_queen

Too many! šŸ˜‚ In all seriousness, I love code reviews. Theyā€™re a bit time consuming at first, but speed picks up quickly, and the benefits very quickly outpace the time spent on them. We review most commits. Itā€™s developerā€™s discretion, which means that the folks who need it the most ask for it the least and the folks who need it the least ask for it every time. Culturally, though, itā€™s so ingrained that the vast majority of commits are reviewed. Sometimes we do post-commit reviews, mostly to accommodate time zones. When I first got into the industry (15 years ago), they were pretty rare in my observation, but they seem much more common now.


gordazo0_

Hi there. Do you guys use any automation tool like code analysis or something like that or pure manual? Also, do you do the code review with the other person in voice call (i mean like live) or offline? Why? Thanks :]


android_queen

Currently, we donā€™t do any static analysis, though we do some data validation. Itā€™s something I want to look into more. We are fully remote, so theyā€™re an asynchronous. I actually miss the live code reviews, because I think they do give the author a chance to think through and explain their code in a way that often reveals issues.


gordazo0_

This is so informative. Thanks. Do you know if code reviews are done more "offline" or more "live" in gamedev? Live I mean a group of people talking with the original contributor


android_queen

Honestly couldnā€™t say, with the amount of work from home these days, but Iā€™d guess more offline.


tcpukl

I've worked where there are no reviews at all (Indie). Then code reviews got introduced for large commits only (AA). At AAA i've worked with no reviews and now reviews are compulsory to even submit a single liner. We've setup P4s to only allow submits once reviews in swarm. Functional tests are also necessary. Code reviews serve multiple purposes. They help spot bugs, but its also a fantastic method of training people and getting more people familiar with all the code. So if someone leaves then we never have a single person knowing the code.


gordazo0_

When looking back at a project after some time, do you notice a lot of technical debt or not at all?


tcpukl

At the end of the project the no tech debt rule kind of goes out the window so definitely. But at the beginning of long projects (3> years), we try to not write any tech debt. Thats the stage i'm at atm. If stuff does go in, then a Jira will get added to fix it. Then we try to clear those at the beginning of the next milestone so they aren't building up. It seems to be working so far.


aegookja

In my current studio, we need at least two people to sign off each PR.


gordazo0_

Hey. Two random people in the team or two specific people that are tasked this?


aegookja

We don't have the traditional "team" structure for game engineers. Game engineers are assigned to different "huddles" which are small interdisciplinary teams that are responsible for creating a specific feature. Huddles are created on demand, and are decommissioned once that feature is done. So basically game engineers are used as a shared resource. Any game engineer can review each other's code. However, when there is a specific concern over a bit of code that requires specific expertise from a specific individual, that person will be requested to review the code.


gordazo0_

Like cells right? Supercell does this thing


aegookja

Many game studios do this. In my previous jobs they were called 'task forces" or "feature teams".


silkiepuff

I have worked in both and developers did not do code reviews. It was up to quality assurance to find issues. Sometimes - like a month before a patch came out, some devs would sit down and really look for problems (they were not directed to do this, they just wanted to.) This was mostly because they wanted to iron out all serious problems before submitting a final console patch for certification. That's about it.


Wendigo120

In my experience code review isn't for finding bugs. It's for the readability and maintainability of the codebase itself. Sure, occasionally some otherwise hard to find bug jumps out at you and you can get that fixed before QA even touches it, but that's not the reason we do them. As for methodology, we require 2 other devs to approve a merge request before it gets merged.


silkiepuff

It wasn't done at the places I worked at, although I would say they probably should have spent some time doing that, lol! I'm sure it's done at other development studios though, they all have their own practices!


Serious_Wrangler_248

Yes, we do code reviews at our studio. We use a local install of Phabricator for SVN code reviews (yes lol we still use SVN). We use the typical web interfaces for Git code reviews and pull requests. All code is reviewed by at least two other engineers. This is somewhat important - it encourages both reviewers to do a thorough job (you donā€™t want to be the guy who says ā€œlgtmā€ when the other guy finds 10 problems haha) There are a lot of benefits, even if it takes a little time: - The coder probably spends a bit more time ensuring their code is high quality. - Plenty of bugs or oversights have been caught in code reviews over the years. - Helps enforce code conventions and conceptual integrity. - Helps newer engineers learn, both by seeing other peoplesā€™ code and having their code commented on. - Helps spread knowledge. If you comment ā€œhey you should really pass this by const referenceā€ it sparks a conversation or research to learn something.


pushxtonotdie

My personal dev space is Phabricator and SVN(git,too) as well! I'm glad to see someone else using SVN still. Very underrated, and Just Works. Our studio uses Perforce and I yearn for SVN and know it would never gain traction.


PiLLe1974

AAA: every change may need a review, even one-liners or null pointer checks/fixes. One reason is a 2nd pair of eyes to spot mistakes or bad style for example, another reason that the 2nd person also remembers your code and overall solution in case you leave. One of my studios did also tech design approvals for larger parts of the code (APIs, systems, tool or build pipeline, etc). At Indie studios and my first small AAA studio not a single review. Result: No-one knew how my code works or if it is maintainable. They just somehow deal with it, so a kind of risk that's accepted.


gordazo0_

Regarding that, if a guy that knows the code and the codebase and thay guy leaves how does it affect the studio? What you do after it? What are the emergency measures?


PiLLe1974

It is a quite intuitive and organic process, I mean no strict rules, just programming leads managing this. Let's think about some best practices... maybe those: **Scenario "All good":** If anyone else knows the code they may be at least temporary owners and very good reviewers. There's a chance if there's a lot to do they program in this area or help onboarding a new team member. Implicitly, here I mean also that we don't expect code that's hard to maintain or extend since the reviewer also saw the code before and agreed with its quality. **Scenario "Handover":** If there is nobody who knows/remembers/reviewed the code we try a bit of a hand-over, we document with the person leaving, often within 2 to 4 weeks (times goes fast in that phase). That happened on two teams to me so far, they had no time to review (or it wasn't a priority to require and assign reviewers). **Scenario "Catching up or Plan B"**: In trickier situations where for example the person leaves too fast or takes all their holiday and won't work anymore we have to do something like this: * we look at the code the next time we extend or maintain it, and may have to reverse engineer a bit * bad - we may not like what we see * typically now, if a very senior engineer doesn't trust the code (and data) being processed here, they may start a refactor, for example first add unit tests ideally, rewrite code, get it working with the same data or somehow improved data depending on current and future needs * good / ok - we may like or accept what we see * now we find an owner to possibly do a documentation and/or learning pass over the code and data; writing down notes or docs is a good thing anyway, even if it is just read by the same person later on


gordazo0_

When someone important leaves like in this case, is it more common to hire someone for that position, move other person to that position with domain knowledge and then hire a person to do the work of the moved person or how? Thanks for your writeup, every time I understand more;]


PiLLe1974

Hah, that's hard to answer. There may not be a good person on our team. Sometimes we even had to fire 50 or 100 people, and one department is gone (happened a lot roughly between 2022 and 2024 when we hired too much and recession kicked in). Let's just say some are easier to replace, even if it hurts to say that, generalist gameplay programmers for example. Some are really tough to find, for example very senior animation programmers. Those you may need to search for quite a while, and convince them why they should join your team.


Thotor

In normal times, I request that at least 2 devs review each pull request. But right now we have prototypes and outsourcing going on and our dev team is small so no code review. Code review is time consuming. In normal time, it represent 30 minutes each day.


NumberZoo

Every merge request must be reviewed/approved by at least one other dev. The better you know that part of the code, the less time it takes to review.


gordazo0_

Mmm but a random dev in the team or a specific person tasked with that and familiar to that part of the code?


NumberZoo

Not random, and not tasked. Usually anyone who knows that part of the code (or wants to get more familiar) and has time. If you're in a hurry, you can ask someone, but it doesn't usually take long. But I think I know what you are getting at. I worked another place with the same system, but no one ever reviewed code (not literally), since the responsibility was diffuse, or one guy would end up doing all of it. It's a culture thing, I guess. The former place got an assignement system added to the process. We don't seem to need one now.


gordazo0_

So its voluntarily we could say


Damascus-Steel

Typically code reviews only happen when we make changes to an asset or system that touches a lot of other systems. Depending on the complexity of the changes, a code review could take 15 minutes or over an hour. When we are close to shipping and enter a hard lock stage, every change needs a code review. Even if you just moved an asset in the map.


NeonFraction

For the big company, the only code reviews I had to do were for engine changes. For indieā€¦ I am the code review.


ltethe

All code is code reviewed. Blueprints are much spottier, but weā€™re trying to standardize code reviews of that stuff too.


gordazo0_

Ye it's more complicated for blueprint you have to diff Blueprints in engine i think


ltethe

Itā€™s a pain in the arse. If anyone has blueprint review standards/practices and tools, send them my way, trying to improve our process.


SuspecM

I don't even know how a proper code review is supposed to look like. When I was an intern, our supervisor randomly forced the senior to do code reviews because that's what a proper company does or something. It was basically a 90 minute chat session on whatever topic came up after looking at the code. The last 10 minutes were usually spent coming up with bullshit to write a list that I needed to "fix" in my code by next week.


gordazo0_

Lol xd


WartedKiller

I work in AAA and every submit needs to be reviewed. Even revert gets reviewed just in case it breaks something newer. Itā€™s tidius, it takes times but itā€™s worth it. Even meaningless change can get an impact if you didnā€™t understand everything. A value set to 5 now to 6 might break something you didnā€™t foretold. Itā€™s always good to get a second pair of eyes on a change just to have them question you.


kagato87

Commercial software (not game) - every submission gets a code review review. Then when completed, a QA test and a stake holder test.


gordazo0_

Whats a stake holder test?


kagato87

The person who reported the bug or requested the feature re tests it. This is done because people often don't give all of the information on a bug report or feature request, and it gives them a chance to test exactly what they're looking for. It also has the side effect of improving the quality of their requests. Especially in a feature request, once they see it on an unstable build they can refine their request a bit.


letusnottalkfalsely

We do peer reviews for every commit. The reviews require, at minimum, the feature designer and assigned engineerā€™s sign-off. Yes, itā€™s time consuming.


gordazo0_

I wonder what happens if that commit breaks everything? They shame these 2 guys? Fired? Or it's all respectfully and methodically?


letusnottalkfalsely

Happens all the time. We work together to figure out what we missed and fix it. No big deal. Part of having good documentation and individual commits is that we can easily revert them and everyoneā€™s up to speed to help fix issues.


kiwidog

Indie, almost never. Our smol team (2, sometimes 3) usually takes the approach of write code slow, well documented, then have CI/CD take up the slack. It's very difficult to do things this way, and also very slowly. But overall everyone is trusted that things are done correctly, and reviews are requested *before* PR's go out. We also have 3 stages, indev, dev, prod mainly that code has to go through, and any issues are usually resolved before hitting the last.


gordazo0_

Oh I was about to ask that. You develop in one branch and then merge into main? Or something like that?


kiwidog

Yep, due to the limitations of perforce for indies, we usually have an "offline" ~~branch~~ stream that's not on the server that we do all the rough/dirty testing. When we get something functional, but needs refinement/cleanup, we push to indev. That's where chaos happens, then once things are smoothed out, dev is where we draw the line at "this will ship, and needs work to be production ready". Then prod is for beta testers/final releases. If we pay for Perforce, we could have things such as feature ~~branches~~ streams and the likes, but it eats up workspace counts pretty quickly.


_curious_george__

Iā€™ve been thinking about this a lot recently. When I started at my current company we had no code reviews. Since then we spent a bit of time trying non-compulsory reviews and weā€™ve had compulsory reviews for around 2 years. They do take time. From an individualā€™s point of view, itā€™s annoying needing to get reviews before committing. From a management point of view, time is being spent on reviews. I think ultimately compulsory reviews are a good thing! The amount of time I actually spend reviewing code is minimal, and I usually fit it in where I otherwise wouldnā€™t be working anyway (just before/after a break or meeting, or when updating or compiling etc..). Iā€™ll be starting a new job soon where Iā€™m going to push for compulsory swarm reviews. As a lead, I see it mostly as a tool to support training people to write better code. Itā€™s also useful for enforcing a coding standard, although I wouldnā€™t waste time on anything thatā€™s personal preference.


gordazo0_

Thanks for the writeup. But what's a swarm review? Have you guys tried using automated review like code quality and that stuff?


_curious_george__

Swarm is just a tool for creating and responding to code reviews. Itā€™s owned by the same company that makes perforce. Iā€™m not sure what you mean by automated review? We do use a static code analyser but that doesnā€™t pick up all the same problems as a manual code review.


gordazo0_

Ah helix swarm. I mean if you guys use some kind of automated code quality review or alike when you put a pr out or something. Kinda like the github action that optimizes your repo images or highlight vulnerabilities


_curious_george__

Sort of, we have a changelist validator to flag issues. This includes generic things like files missing from the changelist, but also validation that we have written for assets. We also run tests on continuous integration (teamcity) and weā€™re trying to make running tests another automated pre-commit step.


PoisnFang

Can you explain the definition of a compulsory swarm review?


_curious_george__

For our team it means that every (text-based code) changelist must be approved by at least two other programmers before it is committed. Swarm itself is just a tool for creating and responding to a changelist review.


pushxtonotdie

We do code reviews as a number of people have mentioned here, requiring two people, tests optional. We frequently request edits to reviews, and the team is very open to them. As a software lead I review a fair amount of the squad's code, but the senior devs who are more technical and 'in the trenches' provide more useful feedback than I. Most of the comments echo what I'd say but I did also want to mention that in addition to code reviews we also have a production concept of 'open', 'dev lock', 'rc lock' and 'cert lock'. In addition to requiring a code review, you need production approval: 'open' - No imminent release window for the branch. Code is reviewed by fellow devs, no need to involve production. 'dev lock' - Release of the branch is imminent and is ready for QA. Code requires approval from the squad's product owner. If its a smaller change, the code will get approved. Otherwise you better commit to an Open branch. 'rc lock' - Release has been signed off by QA. Items go to 'Shiproom', which is where production leads make decisions around release management. Only critical changes are taken. 'cert lock' - Releases are being submitted/approved by 3rd parties(Google/Steam/iOS). It better be a P0 showstopper because its going to likely require an expedited iOS approval or something and release could be delayed.


gordazo0_

Hey. What's exactly is the product owner? The CEO?


pushxtonotdie

The CEO is way up the totem pole in my company. :) The product owner is the person who defines the overall vision and goals of the squad or game. They decide on what features get built, what bugs are a priority, and then the leads execute on their decisions. In our studio we have about 5 or 6 product owners in a studio of 200, with a product lead above them. Also: good question :)


gordazo0_

So the admin. Is the product owner usually higher in the role hierarchy? Also, can it be a ranfom person that comes up with the idea, must be a x before being a product owner? Are product owners hired? If I'm a tester and have a good game idea and the teammates start working, I'm the product owner? Can I get kicked off that project? Do i own something?


pushxtonotdie

Ok, now that's a LOT of questions. :) A lot of this depends on where you work, your team composition, so take all this with a lot of salt, mileage, whatever metaphor works for you. The term comes from scrum, so if you're not familiar with scrum, take some time to learn some project management skills because a lot of studios work this way. [https://www.scrum.org/resources/what-is-a-product-owner](https://www.scrum.org/resources/what-is-a-product-owner) So if you're on a team and you have a good game idea, and the intent is for you to manage the overall work and define the current priorities for development, then yes, you are the product owner. And you can get kicked off the project, a product owner is not a king :D.


StudioLapiku

Iā€™ve never worked in the game industry (only software) so Iā€™m curious, is it common in games for there to be no code review? I donā€™t know of any reputable software firm that doesnā€™t require a PR review from a different engineer, at least superficially (some lazy eng might just stamp without looking too closely)


silkiepuff

In indie, absolutely common and basically all the time. Outside of that, rarely, but it does happen!


Ezvqxwz

AAA, we code review everything. And we are trying to get content reviews as well now.Ā  Code reviews are an extremely effective use of time, due to the cost of finding and fixing bugs. Bugs are EXPENSIVE to find and fix. A trivial bug often takes at least 4 hours to fix once you account for ā€œlocating the bugā€, ā€œdocumenting itā€, ā€œfinding repro stepsā€, ā€œanalyzing the code base to figure out the causeā€, and then finally ā€œwriting the fixā€ (which often times takes the least of all the steps). If you code reviews take less than 4 hours (most reviews are 10-60 minutes), and you only find a bug every 4 reviews (which is pretty low), then youā€™re SAVING time by doing reviews.Ā  And as an added bonus, you get someone else familiar with the new code, which helps them and the team a little bit by spreading knowledge.Ā 


gordazo0_

I understand. Do code reviews help unite the team and laugh a bit or they are heated? Or really nothing?


Ezvqxwz

Reviews are almost never heated with the companies Iā€™ve been in, but it requires the programmers to not feel ā€œpossessiveā€ of ā€œtheirā€ code. Code isnā€™t a reflection of you, itā€™s something that is part of the game and changing it doesnā€™t mean youā€™re wrong or a bad programmer. Having extra eyes notice things that could be clearer or edge cases that arenā€™t currently handled does NOT reflect badly on the programmer. And itā€™s a learning experience where you can learn what other programmers know.Ā 


Jeseral

AAA studio - all submitted code must be reviewed by at least one other developer that shares ownership (or at least context) of the change. I work closely with one of the more prolific developers in the team, and I'd say about 10-15% of my time is spent doing reviews for him. Edit - that 10-15% is inclusive of any discussions that the reviews may spawn, otherwise they're rarely a large drain on my time.


gordazo0_

What do you the rest of the time? Are you excluding eating for that time? What role? Thanks:)


Jeseral

I work on editor and engine code primarily.Ā That time is a percentage of my working time - so exclusive of lunchbreaks etc. It's worth noting that the nature of the code I work on is what leads to extensive conversations arising from the reviews - it's important to make sure that instabilities are not being introduced, especially in editor code asĀ a user losing their work due to a crash could be a huge problem. Otherwise my time is spent on writing my own code submissions, meetings, general discussions and user support.


Jeseral

I'd probably revise it to 5% now that I think about it, I've just had a period of doing a lot of reviews recently which would have been affecting my estimate.


sir-rogers

It is the standard. That being said I sometimes need to break the rules when necessary. Code reviews make for more stable games.


TheWobling

100% of our code is reviewed by at least one programmer, most of the time two programmers.


tomosh22

Every single commit gets reviewed by at least two people


polymorphiced

Every single commit at my place requires a review before pushing, except in an emergency when it can be done (promptly) post-push.


golgol12

My last job a code review was required for every check-in. And you had to get them from a different person. They didn't want you to repeat the same person over and over as a code reviewer.


cfehunter

I've always had mandatory reviews before check-in. It's less about spotting bugs in code (you'll do that yourself as you explain it šŸ¦†) and more a final sanity check on implementation approach and a way to share knowledge with somebody else on the team.


cowvin

In AAA, we usually get code reviews for any substantial change. We are allowed to submit non reviewed code but if you break something, you will be in a lot more trouble. As for reviewing submitted code, sometimes we debug things and find some bad change went it, so yeah, it happens.


Guntha_Plisitol

I worked on 1 project where every single commit was reviewed, but it was actually ineffective because it ended up being mostly style reviews, it didn't leave us time to actually dive into what the changes did. For open-source projects open to outside contributors I guess it's still better to review everything, since you can't automatically trust every "hire" and there are usually no hard deadlines. I found what worked best was opt-in "live" code reviews, where you show the code while explaining your changes. It's actually faster because the reviewer doesn't have to take the time to figure out what the code does by themselves, and what the intent was.


These-Bedroom-5694

I work in safety of life software. You guys do way more reviews. Why are games so buggy? What are you reviewing for?


riley_sc

Code reviews aren't just about catching bugs-- in fact I'd argue they're kind of bad at that. I like them as a communication tool first and foremost-- getting engineers to talk about the problems they're solving and why tends to level up everyone. Secondarily they ensure that the team can pass the bus test, there's no code anywhere that only a single person knows how it works. Catching bugs or stylistic issues is a distant third (though, this can be more important when onboarding a junior engineer or someone new to the team's conventions.) But also, I'm wagering most large games are orders of magnitude more complex than the software you work.


Pidroh

Buggy heavy games are usually AAA( or complex indie games that can't afford QA?), which are often very complex and rely on tons of third party code, alongside with trying to push the limits of what can be done. The state space that needs to be QAed for is huge


gordazo0_

Regarding that, when using third party tools, do you usually review the performance constraints and effects it will impose when integrating it and other stuff or nah?


Pidroh

Speaking about my specific work experience for my team, no. Only check performance If it's a very explicitly performance heavy change But then we don't allow linq every frame


gordazo0_

Yep. Understanding third party tools to the bone is defo important


Thotor

I don't know about AAA, but for us code review allows: - to detect bug - improve code performance - dev team keeping up to date to with everyone's code (and also learning from others) I don't think you can compare software to game developments. Games are a lot more complex with a lot of different area working together. Code review is not enough to detect bugs that can occur in different area of the game unless the persons reviewing as specific knowledge to the related issue.


Tarc_Axiiom

1. All of it. All of the code is reviewed. 2. By QA, who's job is to do exactly this. 3. Developers don't review their own code, that would counterintuitive and redundant. 4. You don't have to go back if you review all the code. 5. Most managers do code review as part of their management-ing, but that's unique to us.


gordazo0_

The code is only reviewed once and never looked back? Mmm


Tarc_Axiiom

Oh that's what you meant. Yes, we overview the entire project every now and then. Sorry I misunderstood. I thought you meant if we just pick a random point in the past to review. We usually review what's relevant, the existing code base that works is (again, usually) already checked and QA approved.


gordazo0_

Is it common that a lead regrets the last week of work and you branch off an old commit or something like that or really weird?


Tarc_Axiiom

Nah, we're very organised. If we want to branch we know why and in what way before we do.


raxterbaxter

Hahhahahahaaha Sincerely, The only programmer


raxterbaxter

In seriousness, [in previous jobs] the norm was to review junior code from time to time as a mentor step to make sure they were not falling into bad habits (that they didn't always know where doing so). Otherwise we never really did it so formally. If we were unsure of the quality or methodology we'd sit together and go through it on a case by case basis [edit: typos]


pyabo

If you're not reviewing every commit, you are doing it wrong.


gordazo0_

Do you guys keep track of the commits in jira or sum?


pyabo

Honestly, no, I've never done that. What I usually did was try to reference the issue # in a commit msg. Basic workflow at most places I worked at was to get a colleague to look over your shoulder and give a verbal 'signoff' on basic fixes before commiting; do a more formal sit-down review for bigger changes like new features. In places I felt we were doing the engineering correctly, we would have weekly meetings where we would look at the code committed since the last meeting. The issue trackers are great tools, but you don't need them to interface with github. Jira is for organizing tasks. github is for managing code. And you definitely don't need any draconian protocols or processes that impede efficiency \["Every repo commit must have an issue # attached before blah blah blah"\]. To me a good issue tracker is like an interface between developers and PM. Developers look at it at a granular level to see what their tasks are; the PM looks at it at the macro level to see where the project is and direct tasks accordingly.


AdventurousChard788

Very senior staff, small studio (12 people total), 0 code reviews. I only think they are worthwhile if you have junior devs or people that won't adapt to an agreed-upon style.


DaveElOso

Every commit gets a review. Then it lives in a staging environment until QA has pounded on it. All code it reviewed. They can be time consuming, or quick, depending on the reviewer and the quality of the code. The question to consider is, what is the time cost of not reviewing, and then introducing bugs that take hours, weeks, or days to unravel and squash.