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 might need to be solved in the future. The future
problem should be solved once it arrives and you can see its actual shape and requirements in the physical
> 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 might need to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe.
I agree so much. Zero-cost abstraction is a misnomer.
> Note how Google does NOT say "make sure the code is properly architected".
is not accurate. The very first paragraph on the same page is:
> The most important thing to cover in a review is the overall design of the CL. Do the interactions of various pieces of code in the CL make sense? Does this change belong in your codebase, or in a library? Does it integrate well with the rest of your system? Is now a good time to add this functionality?
Emphasis mine. But that sure sounds like they care about proper architecture. They just also care about avoiding over-engineering. They're not mutually exclusive.
Of course they care about architecture—I don’t think anyone implied otherwise. But at good companies like Google good architecture is a given. Top developers often fall into the pit of over-engineering, and almost never under-architect. So at top companies code reviewers have to be vigilant about over-engineering and rarely have to worry about under-architecting.
Absolutely not. Google’s interview process and inflow of fresh graduates does not bode well for good architecture. Having spent time at G and FB, I can certainly tell you that employees at both are no better at architecting code in a sane way than SWEs at other companies.
Code architecture requires experience. Google does not.
I've seen more horrible code at a FAANG company than in many other places. Complete absence of overflow analysis in C++, race conditions, architecture astronauts, exploding code bases that no one understands any more.
Features are being added on a daily basis, basically what matters is a high line count.
It’s like requiring candidates to deadlift 500 lbs and then assuming it means they can all run marathons.
- Is `good architecture` a given in this team?
- What are they doing to avoid `over engineering`?
What are the specific questions one can ask to find out the answers to the above two?
For me, I would rather put a bit of additional effort up front gathering enough information about the direction of the project or usage of it to better formulate an attack plan so that it does actually become more future proof. But even that is subject to failure, so what do you do?
Loose coupling, dependency injection, composition over inheritance, and similar techniques tend to be good answers to this question in my experience.
In contrast, over engineering attempts to solve tomorrow’s problems before they arrive and, if they arrive differently than predicted, makes them harder to solve, because when you have to change something it’s less clear which parts of the design were necessary to solve the original problem and which were only necessary for the future problem that was incorrectly predicted. Often times you might end up having to rethink the entire architecture rather than having the relatively simple problem of adjusting things to meet new requirements.
My own experience is that effort invested in removing restrictions and handling corner cases is generally well spent. It may not seem too onerous to have to remember that a particular function works only for nonempty inputs or when called after some other function, but in a large system where many operations have such restrictions, keeping track of them quickly overwhelms the capacity of human memory. Sooner or later someone is bound to forget, and it may even be the author of the code in question. I try to ask "what are people going to expect this code to do?" and then, if within reason, to make it do that (and failing that, to protect it with assertions). Alternatively, someone may be forced to make some other part of the system more complicated to work around the restriction, leading to excessive coupling.
I suppose this practice sometimes risks over-engineering, but I have found the risk to be worth taking. As you say, it makes the system easier to extend further.
To use inheritance as an example - if there’s a conceivable possibility that some future requirement might lead you to add the same functionality to other classes, you probably don’t want to have to deal with a long and convoluted inheritance chain when you could compose some set of functionality onto one class as easily as many classes.
Or in the case of dependencies, wrapping some third party mail sending library in your own generic API is barely more work than using the third party library directly and will pay dividends if you change mail providers in the future.
One very specific example that I worked on recently:
In most cases there are one of X, but in a handful of cases there are many of X. My initial inclination was to branch off and handle the many case as an exception, but then I realized that you could eliminate that corner case entirely if you handled everything as a collection of X, even if in most cases that collection only has one item in it.
So, adding some extra architecture solves it?
For example, in Python, there's no need to go for a class when a simple function does the job. And once you do need a class, it's fairly trivial to swap one for the other, especially in a good IDE.
"Lean" tells you that small batches reduce the waste.
As an analogy, think about looking in front at the road that unwinds passing through cities and attractions until finally it becomes too small to see ahead. You feel that if you stay on the road, it will likely bring you to your destination.
Nevertheless you are not sure. As you move ahead it gets clearer were you are and where you will have to go.
So how far should you go before stopping?
It depends. It is all about how far you can see and how confident you are that it is the right road.
If you do not see too far ahead, it is probably a good idea to just reach the next city at hand and spend some time checking if you are on the right road. Otherwise you may waste time going too far on the wrong path. The further you go, the further you will have to backtrack.
But if you can see further ahead, you want to move faster and skip the pitstop.
This analogy only goes so far. But depending what you are building and were you are in process, and more importantly, how sure you are of what you actually need to build, you may less time in planning and just building what is merely necessary vs building a more complex architecture.
So you may say: "let's try to find as much we have to go and where before we start. But even the pathfinding activity takes time...
Just recently I was looking at Angular, Google's web frontend framework. Services, modules, directives, angular-specific markup. Coming from React, I find this grossly over-engineered.
In general, yes we do practice it. They are guidelines, not rules tho.
Is Angular grossly over-engineered? Depends on how you look at it.
Any code's complexity reflects the complexity of its use case and Angular, for example, needs to fit A LOT of use cases at Google. So yea, sometimes, I think it can be a big hammer for a small nail. In other cases, all that abstraction helps. I guess that's the nature of how code eventually matures over time.
Not really. It’s a sign of too many responsibilities being given to one project. Feature bloat is a real thing.
As code matures it shouldn’t be getting more and more abstraction and APIs, it should be stabilizing with fewer changes. If the former is happening, it’s a sign it needs to be broken into multiple projects.
Breaking something in smaller projects also has its own associated costs.
Leave the code in a state where it's understandable.
ie don't over specify - not just simple things like interfaces, but also things like premature decomposition, that's exposed at the higher level for configurability.
While on the face of it, having everything as small interacting functions means to change any function is quite easy, changing overall behaviour might be very hard - hard to understand all the moving parts, and even worse, if the code split is wrong for future needs, very hard to change.
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.
The programmers I work with can be defensive of their architecture, but they also love to talk about architecture generally. I think there's a "yes and"  way to bring this up that engages the whole team.
Asking because we might have a different process for PRs or a different definition of what it means to submit a PR - on many teams I’ve been on it’s been encouraged to publish a WIP PR in order to facilitate these conversations. GitHub recently added a feature to formalize that but before this we used labels such as “WIP” and “Ready for review”
There shouldn't really be a PR review where the author is seeking input on naming/testing completeness and major design choices. Those two types of reviews are mutually exclusive.
Alternatives to WIPs include whiteboard discussions, pair programming, and written proposals.
Sometimes a prototype can help shake these things out, though. It's often okay to go an extra stage beyond what's been reviewed/approved (e.g., send out a prototype implementation before design review is complete) if you're willing to throw it away. If you'd be upset by someone saying "try this better design", you need to wait for a design review before writing any code.
Another thing to be careful about with a prototype is keeping it as far away from the normal production serving path as possible to avoid compromising the essentials of reliability, privacy, and security. (Ideas along those lines: separate production role, separate server, flag-enable it only in a test environment, dark-launch it, experiment-gate it, etc.)
For example, had a user who wanted a document produced in one specific format. They promised this was what they wanted and it wouldn't change. It changed 5 times as we were nearing release. So I over-engineered it to allow a template to produce the document that can easily be changed (compared to the release process for a code change). It ended up being used numerous times, despite the problem were given clearly states an unchanging template.
I used to be one of them. "Growing up" I realized that striving for simplicity and adding just complexity when necessary is the ultimate sophistication.
Good engineering values and a shared vision of what "doing a good job" means helps out a lot. If we prioritize "development speed" and "quality from a user prospective", extrinsic complexity clearly becomes a burden.
Instead, if these values are not shared, the main motivation for smart people may easily become "ego" and showing off code that may look clever and supporting a lot more use cases but most likely it did not need to be written, wasting time and adding unnecessary complexity with the risk of introducing cognitive overload.
If they can't write idiomatically, are they "very smart?" Code is a communication medium as much as a functional one, and if someone doesn't understand you when you're talking to them, such that you have to say, "well I use a cutting edge grammar, aren't you familiar with Lakoffian generative semantics? Imre Lakatos has a sick presentation on Vimeo," you're not maintaining communication skills. I imagine this could be what "unshared values" means.
The way I see it, it's like driving: people who don't stop at stop signs, who don't use turn signals, who don't know right-of-way rules are not good drivers. These things are a part of driving just as much as knowing where you're going and trying to run the Nurburgring in under 8 minutes. They also demonstrate a concern and respect for the health and wellbeing of your fellow drivers (not to mention pedestrians, cyclists, etc.).
This is not a matter of education, age, or maturity, it's manners. It's saying "please," and "thank you." It's parenting yourself if your actual parents didn't teach you. I'm not sure any of that can be chalked up to surmounting ego. "Ego" is just an excuse and probably not based on actual psychological concepts anyway.
the main motivation for smart people may easily become "ego" and showing off code that may look clever and supporting a lot more use cases but most likely it did not need to be written, wasting time and adding unnecessary complexity with the risk of introducing cognitive overload.
"Very smart?" ;)
I agree with this when it's internal interfaces. When you have public interfaces that you expect to have to support in a backwards-compatible manner for (ideally) years in the future, it's worth taking some time to think about how people might want to extend it in the future.
This assumes that the interface is a function or method call within a binary. If it's an RPC, then making changes is much trickier, since you can't assume that both sides of the call were built at the same commit. This requires a lot of thought to make sure any changes to your RPC messages are both backwards- and forwards-compatible.
"Architecture" is very vague and used a lot of different ways.
I think that is part of the scrum philosophy: management can question whatever it wants and whenever it wants to; if the timeframe explodes that that is the blame of the developer.
Do as I say, not as I do I guess.
Google absolutely want developers that are capable of over-engineering.
Anyone can over-engineer. It’s not a compliment nor a desirable quality.
Why, so, when they get on the job, they need to be told not to over-engineer?
I do believe though that's really hard to discuss effectively as there seems to be no good, and common definition on what over engineering actually is, except in retrospect.
I've seen teams where they were so "good" at avoiding over engineering and "architecture astronauts" that thousand line functions with a Byzantine labyrinth of conditional was preferred to even the most basic of design.
With that said, what would you consider over engineering of the kind that never works, and in what kind of systems?
I don't think this is a good way to do things, but it's mostly what happens IME.
In my experience, projects using Java are the worst offenders here.
Suppose you have a 2^n x 2^n sized courtyard. You have one 1x1 statue, and unlimited L pieces (2x2 with a corner missing).
You would like to have a layout which places the statue in one of the centermost tiles, and fill the rest with L pieces.
Define a layout function with allows for the empty tile to be in any corner. This is trivial for 2x2, as it's just an L peice.
By tiling these squares, you can solve for the next size up.
Put three with the hole in the center and fill in with an L piece to get the bigger square.
At the end, take 4 2^(n-1) squares and put the holes in the middle. Add one L and you are done.
What was the point of that?
By generalizing your solve function to have more outputs, it lets you build up a structure from it, in which it's easy to solve your actual goal.
I would not call them 'very good developers' in that case.
People tend to do it to get promo/bonus.
These days, it’s easier to do a search, grab a snippet, and paste. There’s little need for reusability. Extrapolating a bit, I imagine in a few years, we’ll have one big ‘library’ of functions built into our IDE (built, and copyrighted by Google or Microsoft, of course).
Programming used to be skilled labor, but now it’s kind of dumbed down for higher productivity. A natural evolution.
Personally, I’m trying to re-train myself for the more modern rapid-fire programming. I’m not completely convinced that it results in better products, but it does feel good to be constantly committing.
> 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, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. That most new developers naturally suffer from imposter syndrome is not helped at all by 100% critical code reviews.
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.
- Having your code picked apart is a badge of honor, and you were expected / required to do the same to the most senior of your teammates when they submit code.
- Collective ownership- it's never "your" code, it's the team's code.
- Constant acknowledgement that our system is hard and complex, and it is really important that it works as expected / promised.
That last part was an ingredient I never found anywhere else. The opposite seems to be more common- every other team I've worked on tended to underestimate or even trivialize the difficulty and complexity of the systems they work on. By acknowledging that this work is hard for everyone involved, and that despite this, it must be well-done and function properly, the code review and the ensuing discussions became a very welcome and encouraged part of this process. It also helped defeat newcomers' imposter syndrome because this mentality was effective at making everyone feel like they had an important role to play, and that even the most senior folks often felt like a noob when they screwed up.
This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to appear to the the smartest person in the room, can lead to severe demotivation and stress for both experienced and new team members.
This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand...
I think reviewers should have the awareness to understand not to over-burden the person whose code they are reviewing.
For example, if I’m reading some very junior code and finding lots of issues, maybe I should delete all my comments about minor things like style and only leave the comments requesting major fixes like code design or potential bugs? When the developer re-submits the code after fixing the main issues, maybe it will be less frustrating to get a review back with “this looks great, can you just fix these small issues that don’t conform to our style guide and it’s good to go.”?
I've seen junior developers welcome feedback or run from it, and I think that has as much to do with the reviewer as the author of the code.
On the flip side, I've learned almost as much from reviewing more senior engineer's code and just asking dumb questions or looking up functions in documentation that I didn't know about. I think there's huge potential to learn as the code reviewer.
IMO the first few code reviews should be done face to face so some rapport can be built. Receiving feedback from a 'human' is far easier to process than a faceless Github profile picture.
1. The commit/change is too large.
2. The reviewer is nit-picky.
3. The person that wrote the code made a lot of mistakes.
I really feel like if a reviewer is making 100 comments on a change, they're doing something wrong even if there are a lot of mistakes. That reviewer should really reach out to the person that wrote the code and talk to them human to human.
If I just ended up talking to the guy, I'd have no record and he would have no record (unless he took notes) of all the places he has to fix.
The reason for 100+ comments was a junior developers code and far too large a changeset.
Telling someone that the changes they made are too much for a commit would probably go a lot farther.
Identifying common architectural problems would also reduce what you need to connunicate. Maybe they made a lot of mistakes but I would find it difficult to believe there were 100 unique mistakes.
Automated tools here have also drastically reduced the number of comments I get on CLs at Google, as people don't bicker about the little things as much anymore.
I'm usually okay with preemptively accepting code with only nit comments too just to signal that those comments are not too big of an issue (if at all).
It doesn't help that many comments are nitpicks and pedantry, made by people without much social empathy.
This is especially true when sending a patch for a high-level review of your proof of concept, and the next thing you know people are complaining about your formatting.
"Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that."
"Why is this block of code running even when the condition is false? Oh, turns out the else branch already ended and this part of the code is just indented wrong."
And so on. Even something as minor as `foo ()` vs `foo()` can stand out and act as a constant stream of mental speedbumps.
Claiming your change is high-level / only looking for feedback to the overall design doesn't change that. You're asking for a code review because you want the reviewer to read your code, but reading it is exactly the part that they're finding hard to do.
And it's not excusable, but the reviewer might be insulted that the reviewee is wasting their time and their feedback may be ruder / snappier as a result. After all, the reviewee could easily have put in the effort to run the auto-formatter, follow the existing code's style, etc.
Yes, one should avoid misspelling words when you are about to submit a manuscript for content, but wordsmithing all the the sentences is a mistake.
Likewise, one should run an auto formatter before sending a proof of concept off for review, but a reviewer who is all tied in knots about low-level nits when looking at a general proof of concept is editing at the wrong level.
Otherwise you end up with a fully-baked, carefully written solution that satisfies all the nits, but was a giant waste of time because it takes the wrong overall approach.
On balance, formatting is an important part of code health. Hopefully, using auto-formating tools (gofmt and friends) should hopefully make code formatting issues not take up time in code review.
Many people often think their skills and actions are them, instead of things they've acquired.
The question you're asking is oddly similar to asking why people get stressed and mentally suffer at all. It's a deep and complex subject. Taking is personally only scratches the surface.
Eg, I know someone who switched her career over it. I asked her why she left and she told me they kept criticizing her, and she took it as if they were attacking her and trying to get her to quit.
I was the primary architect and reviewer for a complex real-time mathematical application. When reviewing code, I was pretty much a tyrant: the code had to be correct, well tested, conform to the theory, interface with the rest of the system correctly, etc., in order to be allowed in. I remember leaving some pretty brutal reviews when the proposed design was different than what I thought it needed to be. I thought I was doing the right thing, the project lead thought I was doing the right thing, but maybe I was just making my teammate's lives hell.
In a subsequent job, I was on the receiving end: I was again the primary architect and maintainer, but still needed to seek review from a larger team (my component was part of a larger project they owned). The experience was not enjoyable: reviews sometimes took months (I sure wish I was kidding), sometimes were passthrough "LGTM! I don't understand it at all!", sometimes asking questions like "why is this mutex here" and then I have to spend 3 hours writing up an explanation for how threads and locks work in this case. I found that my mental model shifted: instead of committing small improvements here and there like cleaning up comments or renaming something I just... didn't. I didn't want to deal with a multi-day process of bugging someone to review (they were always busy), dealing with the roulette wheel of comments that might come up, the possibility that I might have to justify some minor thing that I don't even remember the reasoning for. It felt like making a PR opened you up to an uncomfortably invasive inspection, one where the reviewers look down their nose at you and ask you to elucidate why you chose to wear the red shirt today instead of the blue one, as if you're supposed to have some grand unified theory of shirt colors when the actual reasoning is "I thought red would work and it did". How are you supposed to justify why you didn't do all the things you didn't do?
I think an issue is that there's always a different approach that could be used and in a perfect world perhaps we'd iterate endlessly until we found the best one. I've seen plenty of systems that have a design very different than what I think I would do, but as it turns out those systems work too.
I've honestly become less convinced that code reviews are the answer. Is there a possibility for learning reviewer <-> submitter? Of course. Do some teams find code reviews to be hugely beneficial? I would assume so. But I don't know if an organization-wide mandatory absolutely-zero-exceptions is the way to go.
Reviewing can thus be experienced as opening up yourself to critique by essentially anonymous "kingmakers", which at any time can make you look like a clown, if they want to. This experience might not be rational, but I believe it's still extremely common, as an experience.
It's made worse by the fact, that if the review culture is anything else but great, it's not at all unlikely to try and take advantage of that - in one of many ways - in order to feel a bit better about themselves.
At our core most of us are social creatures, and thus most of us easily pick up on that danger/risk, including most that are otherwise not socially adept, as they have usually suffered bullying of some kind and have learned to avoid those exposed situations.
When you've been working for a while you can filter out the noise regarding formatting and other smaller issues (or, ideally, get autoformatting set up), and it gets easier to separate your ego from your code. When you're new though, and showing someone else your code for the first time it's hard to see a huge volume of what seems like substantial negative feedback on your code (and by extension yourself).
I realize the "right" solution here is that people shouldn't equate criticism of their code with criticism of themselves, but I think that's a huge thing to expect at first and contributes to making the field unwelcoming.
When the reviewer communicates this to the coder, this often comes across as criticism of code that is in reality perfectly fine.
It's much harder to power through (2) than (1).
Unfortunately many people stink at communication. The best thing a reviewer can do during a code review is ask questions. A reviewer who comes in and just says 'this is wrong' misses an opportunity to either learn themselves or actually teach the submitter. A much better approach is to ask the submitter why they did something certain way. What were their thoughts, and what did they see.
IME, asking questions leads to much better outcomes. Sometimes the submitter will just admit they were not thinking about anything and see their own errors. Upside, they found the issue themselves. Other times they'll explain some complexity or use case only they could see while deep in the code. Upside, I just learned something.
In general the best teachers ask questions. It doesn't put people's egos on the defensive, and tends to help people learn more effectively (the goal if any review IMO). I've seen this work on me in other scenarios like Jiu-Jitsu. My teacher will ask mid-roll or afterwards why I did something, no matter how stupid it was. Explaining my thought process helps me to better recognize the error, and I'm not immediately on the defensive. I'm then much more receptive when the teacher goes "that wasn't a terrible idea, but if you did this...".
Personally I try to make them often, but I also try to be thoughtful about making them as to not come off patronizing like you mentioned.
Though I’ve never felt like a positive comment ever made me feel patronized in any situation, I think a good way to not come off patronizing is to to make the comment more personal than general, ie:
“This is a cool way to use the rest operator (or whatever thing), Ima steal this”.
Obviously it needs to be an authentic comment, but I think it’s a significant difference vs
“Great use of the rest operator!”
When the interpretation on the other end might be “oh so this person is surprised I know these basic language constructs”.
I learn things from doing code reviews all the time, and I like letting my coworkers know that I feel like I’ve gotten better at my job as a result of reviewing their contributions.
I think it’d be a shame if I didn’t let them know.
Sometimes, it is worth the risk of sounding patronising.
Not really. Positive comments help teach engineers which things they've done that conform to local best practices (and why) without them having to meticulously dig those up (assuming they're even documented). A lack of positive comments leaves engineers to learn them only by running afoul of them. Effectively it provides direction only when some threshold of badness is crossed, while leaving positive comments on good code (especially for new team members or junior engineers) provides a beacon pointing away from the badness threshold entirely.
Put differently: commenting on good code makes for swifter and less eventful code reviews by steering engineers away from the bad practices that make code challenging to review in the first place.
Also, it's just nicer to spend forty hours a week with people who demonstrably appreciate their peers' good work.
The only positive thing to report should thus be that all is fine if the reviewer did not find any issues.
Now, if you are conducting the review in a meeting then obviously you can make oral comments in passing. If you're using a software tool for reviews then the all the comments should be on point. Nothing prevent you from talking to your coworker afterwards to spread some love if you want to.
(Convoluted comments should be made more concise)
As said, if you want to praise then you are free to do it offline.
This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school... I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 years.
Citing a good practice in someone's code as "yes, please do more of this" alongside "don't do this please" is not, in my opinion, fluff.
> I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 years.
So only pay attention to unresolved comments?
That simply isn't the purpose of a code review.
Good practices should be documented externally, so you can check them consistently during code review ;) It's also quite useful to have a checklist when doing a code review.
Communicating positive feedback is crucial for teamwork, teaching, and passing ideas on.
Nurses don’t compliment each other for using a new pair of gloves on each patient.
Feeling valued is linked to job performance. And if I notice any of a) a generally interesting piece of code b) the engineer being a boy scout and fixing something in that particular module to make his changelist better c) a novel/comprehensive way of testing the code automatically d) elbow grease to just go the extra mile in terms of doing a great job (without adding unnecessary complexity) I will call it out in the code review along with the regular 'fix this' feedback. As a principal engineer my word carries some amount of weight and I truly want the person to feel good about their work when they deserve it.
Another thing I often do is add a 'thank you for taking the time to do this' whenever someone slightly decreases the amount of tech debt (by refactoring, or removing code/complexity) when it was obvious it wasn't absolutely necessary to get their work item completed. Basically whenever someone shows they're thinking strategically rather than tactical about their work, I want to make sure that person knows that I noticed and that I value that.
People aren't robots, we're all professionals and we all like to feel good about our work.
I got 21 comments to remove blank lines and none about the code. Thanks. Really useful. This was the team of "experts" on the code base.
Then a principle engineer reviewed some other code and oh look, actual useful comments.
Nitpicking can be worse than useless. Obfuscates or ignore real issues in the code. Seems to usually be a sypmtom of the reviewing not being competent enough to actually review the code properly.
Trivial comments are a sign of that.
Review code in this order: protocol buffers, unit tests, headers, implementation. It's common for a new employee to be an expert on C++ or Java or whatever languages but it's very uncommon to meet anyone who knows how to define a decent protocol message. The tests should give the reviewer a very clear idea of what's happening in the code (and this should be congruent with the description of the change), if not send back to author at this point. The headers should contain clear interfaces, types, and comments and should not contain anything that's not part of the API (when this is technically possible). Finally look in the CC file; at this point the reviewer should see things they were already expecting and no funny business.
Any claims about speed, efficiency, or performance whether in comments or during the review must have accompanying microbenchmarks or they should be deleted. Wrong ideas about software performance abound, and even correct beliefs become incorrect with time, in which case the microbenchmarks are critical to evaluating the continued value of the code.
When sending a changelist for review, always clear all automated warnings or errors before wasting the reviewers' time. Nobody wants to see code that doesn't build, breaks a bunch of tests, doesn't lint, etc.
1) Decision to use some kind of 3rd party library that purportedly speeds up handling primitive types (Trove specifically. Almost never worth it, and it's extra painful because it doesn't plug in well into the rest of the standard library).
2) Using mutable objects to avoid the inefficiencies of allocation and copying with immutable patterns. Can lead to a brittle stateful design with hard to track down bugs.
3) Reusing serialization artifacts across the call stack to save copying/allocation, again like 2). Now you end up coupling the API interface to layers deep in the call stack making things harder to modify.
Also best review advice I got and follow: be the first reviewer yourself! Looking at a nicely formatted diff really lets you see your code in a new light, and usually I end up doing several iterations before I run out of things to tweak. Things like forgetting debug log statements or commented out code, or dev settings, as well as other easy but effective improvements.
I've been enjoying lots of comments in this thread, but unequivocally THIS. My company (for no particular reason) has two code review tools. Each team picks which one they like. Early in development I'll shelve a CL into the tool my team doesn't main, and just review and annotate diffs. It's a good place to keep TODOs and notes to myself.
Additionally, if I share it with my team, its clear that its "pre-review" quality and not to nitpick. "WIP" in the subject line helps, but being in an entirely different application really solidifies the point.
Then, when ready to go into real review, I've got a really good sense of how the CL looks as a diff, and where any remaining annotations need to be placed (that aren't truly a comment).
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 really critical part of the code, then sure, write a benchmark. Outside of that though, it's rarely a good use of developer time.
I came across a comment in google base libraries that said "This is faster because cache line is 32 bytes". It had been written by a very famous engineer, and it was even true in the days of the Pentium III processor. But at the time I found it it was not only false but the code as written was slower on modern CPUs than the shorter and totally obvious equivalent.
You would want to ban that sort of comment?
If you see a piece of code written oddly, then someone telling you why they wrote it that way is very helpful. If later you have to refactor it then it's doubly nice to know the reason it was written that way doesn't apply any more.
I suspect your problem is with pre-mature optimisation rather than commenting, and if so I imagine the majority of programmers would share your views. But if that is the case banning comments that make it plain something may have been pre-maturely optimised doesn't seem like a good way of solving the problem.
If it isn't worth your time to write a benchmark then it isn't worth my time to change the code.
Everyone thinks their own performance tweaks are simple and obvious, but I've rarely seen measurable performance boosts from comments in a typical PR.
But if you're attending cppcon, two engineers in Google's production toolchain organization are giving a talk on "Releasing C++ Toolchains Weekly in a 'Live at Head' World" where they'll discuss (among other things) how we use various microbenchmarks to catch performance regressions.
BUT I think a more important consideration is readability and clarity. Make it work, make it pretty, make it fast - in that order. If there was anything I learned from a Go course some time ago (Ultimate Go iirc) is that speed is a natural result of readable code. Plus if your code is good and well structured it becomes trivial to benchmark, identify and resolve any performance issues.
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.
In this particular case I'd expect the change to be motivated by profiling of the real production instances. And that makes it pretty obvious how the change should be evaluated. "We're spending more time than is reasonable in linear scans, so the worst case inputs must be worse than expected. Switch to a data structure more suited to large inputs, and see if CPU use improves in the next rollout."
No one here is agreeing with that.
Generally true, but not when prototyping. As a reviewer, I would like to see the general idea before spending time on cleaning up. If the grand design is wrong, time spent on cleaning up would be a waste.
CL: Stands for “changelist,” which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a “change” or a “patch.”
Code reviews are mostly in the social domain and not the technical domain. Other than bugs and performance, everything in a typical code review is an opinion. Hence, you need guidelines on how to handle differences in opinions. Different work environments structure code reviews differently, but often the worst is where a reviewer finds problems, and the code review cannot be marked complete until the reviewer is satisfied. This often leads to "I have to change my code to the reviewer's preferred style" instead of "The reviewer found real issues". Other models, exist and are better.
 Actually, performance conversations are very often based on opinions. Things like what level of performance is acceptable, etc.
Which of course is not reality. I mean I get it, I too would, I think, love to be a hero developer and just immerse myself into code for weeks on end, but that's simply not the reality. I don't write code, I contribute to a customer-facing website / shop, and at the moment my time and energy would be better spent adding a date picker than to fantasize about rebuilding the back-end in Go.
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.
I had many group projects in school, but they didn't set it up so that we'd review each other's code. We'd just break it up into modules, split the work accordingly, and each worked on a different part.
A more fundamental limitation of school projects is that they have by definition a limited lifespan, so that the notion of code debt, the importance of ensuring that code be readable by current and future colleagues, etc. are irrelevant - in all the projects I've done in school, even group ones, I was the only one reading my own code and it would never be run after class finished. In such a situation, "improve overall code health" makes no sense. You write only what you need to pass the class, which is usually some kind of demo feature.
Maybe making contributions to well-established open source projects should be encouraged as part of the curriculum, at least to experience the receiving end of it...
Yes. Dorm life is supposed to provide the social aspect. And to a lesser extent, real group assignment/projects.
This is a great rule of thumb
Imagine some new features such as “support exporting to CSV” or “auto-generate avatars for new users”. Implementing them will necessarily add more code, and more complexity, to the codebase. And adding more complexity makes the code harder to work with – in other words, harms “the overall code health of the system”. Businesses develop new features because of the improvements to the product they bring and the reputational or monetary gains that come from that, in spite of the damage done to the codebase. The only way you could get a new-feature CL to pass by the standard you quoted (the standard of “improving the overall code health of the system being worked on”) is to refactor existing messy code written by someone else, and that would not be sustainable.
Does anyone see a way to interpret the rule, or have a preferred variation of the rule, that makes it useful for evaluating new-feature CLs?
Mind you that was before Prettier, nowadays a lot of these niggly things are automatically identified and fixed in editors and a pre-commit hook.
> It is the end of the day on a Friday and it would just be great to get this CL in before the developer leaves for the weekend.
I laughed out loud because it reminded me of so many times I have seen it happen and then someone had to fix in the weekend.
Who shares the same experience?
Strangely they also list as "not an emergency" rollbacks of clearly broken code, but that's an exception to review rules inside Google. Anyone can do a pure rollback of a change without getting the approval of the owners of the code, and there are automated tools that will roll back changes that appear to have broken at least 1000 tests, without human review.
Edit: There's even a button right in the code review web UI to roll back a committed change.
As long as everyone admits it, sounds like those are actual emergencies :)
The threshold for this seems at least a little too high, doesn't it?
And they really want to, because they don't want to have to do a bunch of merges when they come back...
After they started enforcing that they got a lot more productive and started shipping on time much more often. Turns out the mad rush to meet an arbitrary deadline absolutely kills productivity.
1. Higher throughput, higer latency and correctness on one side.
2. Lower throughput, lower latency and some risk of faults on the other side.
It's like the difference between tcp and udp.
The question is: Where do you need to be? What are your requirements?
The logic was we have fewer users over the weekend, so if something goes wrong, fewer people will notice.
Some who administers some SAP thing for a huge supermarket chain tells me they do all of their upgrades on weekends because their users aren't at work.
Makes sense for them, but it would be a crazy thing to do the consumer-focused interweb company that I work for.
I justified as saying if something breaks we have everyone there to fix it. Plus I'm not going to spend my weekend working if I don't have to.
My app has global traffic. Sometimes stuff breaks Sunday night and Asia is the first to find out.
The test of a good cd is that your ok with releasing on Friday at end of the day.
There is only one way a group of people can have healty code reviews, that is when they like each other so much, or enough to accept what they not agree on. But man, if one or more dev's are not you're best friend, you have a problem there. Because it is not who has the best code or ideas, it's who fits best in the group, not?
But I totally agree code that comes into a code-base needs to be checked. And IMAO only by 1 developer, the LEAD who is responsible for the code-base. No democracy, no endless discussions, no 6 people spending 1 hour a day to do this.
Personally I prefer to do only gigs where there is a lead dev. I try to stay away from the popular self managing teams. I've seen enough horror, where a good developer had to leave the team because they were constantly frustrated, endless discussions about futilities, and in the end it all became personal.. Is it only me who sees this?
Yes, acknowledge when someone means “micromanaging” when they say “agile”, and act accordingly. But don’t redefine the word in your own head, otherwise how can you even speak?
Like, what word do you use now for the basic principle of agile development now that you’ve changed it to mean micromanagement in your head?
1580s, in Aristotle's logic, "a highest notion," from Middle French catégorie, from Late Latin categoria, from Greek kategoria "accusation, prediction, category," verbal noun from kategorein "to speak against; to accuse, assert, predicate," from kata "down to" (or perhaps "against;" see cata-) + agoreuein "to harangue, to declaim (in the assembly)," from agora "public assembly" (from PIE root *ger- "to gather").
Now, if you've been fortunate to work in places that more closely adhere to the original ideas and concepts that's great; count yourself fortunate.
A Philosophy of Software Design by John Osterhout is a good book on these questions.
It's mostly a "I know it when I see it" kind of thing. We know most of the time what bad naming looks like - it creates confusion, tells you nothing about the variable ("i", "n", "val", etc). I guess this is not a science (yet?) and we're currently at the stage of defining anti-patterns, out of which maybe we'll figure out some actual desired patterns.
- Each night, I go over all the commits of the day and do a code review
- Each morning at 9:00, we go over all the comments on the commints, with everyone, talk about it and make sure everyone understands.
This allows me to explain more advanced or new concepts that one developer uses to make sure everyone understands. It allows me to introduce improvements and explain why they are beneficial, and it allows me to signal when people don't follow the standards we impose; giving not only the person who did it wrong a refresher, but it makes sure everyone is reminded of the way we should write code, etc.
The feedback has been amazing and everyone loves it because it helps everyone grow and they feel like they are allowed to focus on quality; due to time pressures, coders tend to let standards slip when under pressure. When you focus on quality every day, it stays with them, because they know, if they don't deliver quality, it will be in the daily review. So far, this has not hurt production speed at all and quality has gone up a lot since we started doing this.
This is a problem I see far too often but it’s rarely talked about. Too often, engineers misinterpret “quality” code for “their” code. Code review turns from “what should we be doing here?” Into “what would this reviewing engineer name this one unimportant variable here?”
There needs to be a happy medium between velocity and quality, and increasing velocity doesn’t necessarily mean decreasing quality.
Hiring someone just to review your code is not a sane business decision, so you might want to think about how you overvalued that aspect. I would be very surprised if someone could make a business out of 3rd party code reviews, but stranger things have happened.
Depending on how you define "make a business", this already happened. There are paid for code review and vulnerability scans. Sadly, I can't remember the companies that did them. I think one of them was by IBM... I saw them applied to new software (when it was nearly done) at two big, European companies. They were mostly worthless: The insights were barely above what Sonar gives you and many findings were "never gonna happen" edge cases.
Code is a bit like music, there is very little right or wrong, but a lot of flavours and opinions.