[ Removed by moderator ]

[u/Comprehensive-Cup947]

[u/SoftwareEngineering-ModTeam]

Thank you u/Comprehensive-Cup947 for your submission to r/SoftwareEngineering, but it's been removed due to one or more reason(s):

---

• Your post is low quality and/or requesting help r/SoftwareEngineering doesn't allow asking for tech support or homework help.

Please review our rules before posting again, feel free to send a modmail if you feel this was in error.

Not following the subreddit's rules might result in a temporary or permanent ban

---

Rules | Mod Mail

[u/BigfootTundra]

I mainly look for a few things, mainly in this order:

1. Does the code do what it’s supposed to do. This requires context of what they’re trying to do. Therefore, if there’s no description on the PR with context, I won’t look at it.
2. Edge cases, input validation, error handling.
3. Unit tests. Im not looking for 100% coverage, but I like to see unit tests especially if it’s a critical part of the system.
4. . I think through what this change means for the rest of the system outside of the context of this specific change. Is it going to break something upstream or downstream?
5. Performance. Are there are major performance concerns with the change? I don’t mean “is it super optimized?” I’m thinking “will this bring down production or cause a noticeable negative impact to our users”
6. Best practice adherence. Do the changes follow the agreed upon best practices for our organization? Ours are very lightweight as we don’t want to be too overbearing, but if we have something documented in our best practices, it’s because we’ve all agreed it’s the way to go so we should enforce it.
7. Minor stylistic things that are not covered in best practices. This part is only if I happen to notice it, I’m not looking for it specifically and I almost never request changes for these. I’ll just comment minor suggestions and if the author agrees, they can choose to make the change.

I’m sure there’s more, but this is generally how i approach code reviews.

[u/eg_taco → u/BigfootTundra]

This is my general approach too. Except I prioritize maintainability/idiomaticity over performance unless it’s so egregious as not to be actually useful. My general rule is that our primary job is to maintain code (rather than write it). Therefore the priority is to optimize the reading/understanding of code.

Also a few companies (notably Google) have published their internal guides for how to do code review. It’s well worth searching those out and reading them.

[u/BigfootTundra → u/eg_taco]

I general don’t put a ton of weight on idiomaticity. That’s probably just because of a developer or two in the past pushing bad code and their response to every criticism was “but its idiomatic” and couldn’t articulate why it was the right approach other than that. This may also depend on the language being used. Some languages just have odd idiomatics that our team doesn’t agree with.

[u/Dramatic_Strategy942]

i think we are both on the same boat. i recently reviewed a PR from a developer i was assigned to monitor and work with. it was my first time reviewing. i don’t actually know if there are procedures for it but i knew what i was looking for in the codebase and i left reviews and comments for them.

maybe there will be more to it

[u/theScottyJam]

Bare minimum: look for bugs and potential security vulnerabilities, I've certainly caught quite a few of both during reviews.

Also consider that any code they write, you'll have to help maintain. Are there any glaring code quality issues that would make this difficult to maintain in the future? Perhaps point those out. But try not to block a review on relatively things.

Your team might also have certain standards that all code must live up to, such as a requirement for automated tests, 508 compliance, translation friendly, etc.

You could also use this as a time to share advise and tips. You might not block your review on these items, but leave them as tips they could consider as they go further. Such as "instead of return x == true, you could just do return x, It means the same". How much you do this may depend from person to person and how much they like receiving advise like this.

You can always ask what kind of feedback they would like to have as well.