It's very interesting that so much of it is about the social aspects of engineering - a lot of it kind of reads like "don't be a jerk". In CS engineering classes, where presumably we would learn to become great engineers, I don't recall learning about any of this, and instead I remember the emphasis being on technical knowledge and accomplishment. I'd probably have been a better engineer in my early career if I'd understood how much the ability to exchange clear, constructive, and nonthreatening critique with peers actually mattered. It's reminiscent of how training in technical fields such as architecture and medicine often put the emphasis on technique over the service aspects of the job such as client interactions and bedside manner.
How to do a code review
151–160 of 376 posts
Re: How to do a code review
#152Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…
I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.
Yes! Engineers should collaborate on the overall approach to a code change/addition with the reviewers before any PR is submitted. Those early discussions are extremely important since teams will converge to an appropriate solution quicker because the process is much more informal. These frequent early discussions also build team spirit, and if you're so lucky, individuals will start to click and the team will start writing code and designing software in similar ways. These early discussion are generally more jovial. The actual review can focus on details, and, if feedback was gathered and put into practice early, nobody will mind the occasional nitpick about punctuation and naming. These will feel earned, even welcome, the PR being a final touch of polish.
Re: How to do a code review
#153Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…
This is for a code review which would be well after architecture and be the entirely wrong place to systematically question architecture.
My #1,2, and 3 rules are to get the code in the right place. You could vaguely say that is architecture related--is it in the main app, a library, etc... Those decisions can be tough for junior developers and can often be disputed by seniors (we don't need a library yet.)
"Architecture" is very vague and used a lot of different ways.
Re: How to do a code review
#154This is great advice and isn't followed often enough, especially when reviewing code written by people new to an organization/team: > If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, i…
At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.
People call positive comments noise? They put scare quotes around “positive”? This field really scares me sometimes.
Communicating positive feedback is crucial for teamwork, teaching, and passing ideas on.
Re: How to do a code review
#155Here's the part that resonated with me most: A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates…
I've been pushing the idea that if you're getting meaningful feedback on design (and over design) in your PR reviews than you've failed. That stuff should be shaken out long before you have working, complete code.
Is that a productive idea to push?
Not to say you're wrong, but some hard human problems to solve are:
- getting people to not be defensive during code reviews
- getting people willing to be critical (constructively) of their peers work
Emphasizing that code reviews with productive design comments indicates a failing seems more likely to stop the comments, not to improve the design. Most people wont want to do the work multiple times and will quickly learn to do as you desire in the face of repeated code reviews that have comments pointing out design problems.
Re: How to do a code review
#156One of the things not being discussed on this thread is all to the requirements around the side of the commit and how that impacts everyone else. I wonder when companies or projects get mature enough for these types of practices to apply. I know google has a complex lifecycle for projects internally as well.
Re: How to do a code review
#157Earlier quoted context omitted.
I once worked on a team that specialized in very long littanies of code review comments... but they were able to bake this into their culture in fundamental ways such that it ended up being one of the most positive experiences in my software engineering career. The basics of how they accomplished this was: - The obvious- no personal / destructive attacks or insults, no cussing, no comments on any person's abilities.…
Sure, but “Hey I really appreciated how you did this thing here because it’s tidy / does thing X really well / takes into account future whatever” never hurts to throw in either!
Yup, I failed to emphasize how much I agree with that as well!
Re: How to do a code review
#158Earlier quoted context omitted.
> Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. That's way too strict. If I want to suggest moving a statement that's inside a loop to the outside, I don't want to waste my time writing microbenchmarks showing that it improves performance. If the suggestion is complicated or non-obvious, and it's in a reall…
I agree that "must" is too strict. If I replace linear search with binary search, or hashmap/hashset lookup I don't need to write a benchmark to prove it improves performance. There is math, logic, Big O analysis that allows to reason about performance and speed without microbenchmarks.
bt848 already said it, but you picked a really poor example. In many real world scenarios, a linear search beats a binary search due to a lack of overhead.
Along those lines, in C++, a vector very often performs better than a theoretically better data structure. I learned C++ fairly well from a very experienced guy in the company, and he said that you should always benchmark against a vector. I suspect if I was in his team and he were reviewing my code, he wouldn't let the code pass unless I had benchmarks that show whatever data set I picked is faster than a vector.
He wasn't against other data structures - he just wanted proof they would perform better. In quite a few cases, they didn't.
Re: How to do a code review
#159It's very interesting that so much of it is about the social aspects of engineering - a lot of it kind of reads like " don't be a jerk ". In CS engineering classes, where presumably we would learn to become great engineers, I don't recall learning about any of this, and instead I remember the emphasis being on technical knowledge and accomplishment. I'd probably have been a better engineer in my early career if I'd u…
Code review is a social activity and when you frame it as such it’s not so surprising that a lot of the discussion of it involves these social aspects.
I definitely agree that my experience was the same w/ regards to an almost exclusive focus on the technical aspects of programming. I only had one CS project in my senior year that involved building something with another person and even in that case it was only one other person.
I don’t think this problem is simple to solve or specific to programming. On the one hand, educators have to asses individuals so it makes sense that they’d want to isolate work to the individual. On the other hand, work after education is never (I hesitate to generalize but this seems like a safe generalization) an individual activity, and the rare dreaded group project in school is the norm afterwards.
I am not sure what the solution to this problem is. Even in a situation where you have version control and can look at each individuals contribution to the code itself, unless 100% of the discussion around that code happens in comments on a platform like github, it’s still difficult to assess things like how much the input of one individual contributes to the output of another.
In addition to this, teaching effective approaches to technical problems is easy compared to teaching effective approaches to human interactions.
Re: How to do a code review
#160[deleted]