Just be prepared to say NO, this is what a good reviewer is all about: https://www.yegor256.com/2015/02/09/serious-code-reviewer.ht...
How to do a code review
291–300 of 376 posts
Re: How to do a code review
#292Earlier quoted context omitted.
>Why is it that people feel so discouraged by loads of review, especially early on? 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…
I can offer a personal anecdote here on my experience with code reviews, on both sides. 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…
I share this sentiment about code reviews. Too often, they are time consuming discussion over trivialities and/or taste. Also, like you said, it's way to easy for someone to ask one-liner questions which require long and time-consuming explanations. I would be fine with doing code reviews with people who I consider reasonable (ex. I hand-picked them for a team). Otherwise, it is often a drag which doesn't improve the product that much.
Re: How to do a code review
#293Earlier quoted context omitted.
The quote, for people on mobile: > 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…
Thanks! I agree so much. Zero-cost abstraction is a misnomer.
isn't "zero-cost abstraction" shorthand for "zero-performance-cost abstraction"? i.e. computer, not human, performance?
Re: How to do a code review
#294Here'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…
When you have very smart people, it may be difficult for those people to write simple stupid code. Smart people like to write delightful, sophisticated and elegant solutions that will address problems and use cases that the code will never go through. A library may be an exception.
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.
Re: How to do a code review
#295Earlier quoted context omitted.
> How at this point code review not entirely automated? There is no automated way to check intention. I can write perfectly correct code which also drops the database.
In this case, wouldn't you use some kind of permission/security groups/roles at the db level or firewalls that would block such intents? Wouldn't it be possible to add an automatic linter pass that does "grep DROP" on the source code? Even unusually things such as code obfuscation can be automatically detected to some extent. An automated way sounds more fair, robust and scalable. To me, there is something in human c…
Dropping the db just an extreme example. There are infinite ways a developer can use tools correctly but build wrong things. Don't get me wrong, automating things is great, but you're never going to replace code reviews. Unless you have some crazy advanced AI that can interpret requirements and detect incorrect implementations. But at that point I would imagine the crazy advanced AI might as well be creating the implementation in the first place...
Re: How to do a code review
#296Earlier quoted context omitted.
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 hi…
Yes, the size of the data matters, and linear search may be the right choice for many use cases and may outperform more advanced data structures. You learn it on the job as it is not a topic usually studied in universities. But I don't agree that we need to completely throw away theory, O complexity analysis where N - input size is the main variable. Microbenchmarks are hard to write correctly. Recreating the scenarios occurring in production is not always possible in the microbenchmark settings. You can write a microbenchmark that shows your code works great, but fails to perform in production because the input data is totally different. bt848 mentioned load test, which is not the same as microbenchmark. Load test that simulates production settings is a better tool to validate the optimizations.
Re: How to do a code review
#297Here'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…
But, we should also mention that Google also has some of the best software designers and architects in the world. You can't get a well-engineered library without someone experienced at design leading it. All of Google's open source projects have significant design. And, if you look at the commit logs, you can see that their designs didn't emerge from thousands of little pushes. Someone led those projects and enforced design criteria from the beginning.
Re: How to do a code review
#298Earlier quoted context omitted.
>But at good companies like Google good architecture is a given. 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 think the hiring bar at places like Google and FB is astronomically higher than almost all other places - they definitely have generally higher skilled people there on average.
Checking for memorized algorithms and an MIT degree do not guarantee good code.
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.
Re: How to do a code review
#299Earlier quoted context omitted.
It sounds like walking a tight line between over engineering, and falling into technical debt. If you design code specifically solves the immediate need, that may need to be thrown away or extensively worked on / around when future needs come up. On the other hand, you can write code that solves future needs that never appear, and still fail to solve the actual needs that end up appearing. For me, I would rather put…
I think of it this way: How can I build this so that it only solves today’s problems but doesn’t make it overly difficult to solve tomorrow’s problems? 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…
> Loose coupling, dependency injection, composition over inheritance, and similar techniques tend to be good answers to this question in my experience.
So, adding some extra architecture solves it?
Re: How to do a code review
#300Here'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…
It sounds like walking a tight line between over engineering, and falling into technical debt. If you design code specifically solves the immediate need, that may need to be thrown away or extensively worked on / around when future needs come up. On the other hand, you can write code that solves future needs that never appear, and still fail to solve the actual needs that end up appearing. For me, I would rather put…
Your goal should be to get to the destination as fast as possible.
"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...