← Back to context

Comment by mparnisari

2 years ago

His book is awesome! One takeaway I had: code reviews matter. If your code is undergoing review and a reviewer tells you that something is not obvious, don’t argue with them; if a reader thinks it’s not obvious, then it’s not obvious.

11 comments

mparnisari

Reply
DoingIsLearning  2 years ago

I think the issue is _when_ you make the review. Psychologically suggesting a breaking change after someone invested in a bunch of tests and verification will raise their defenses up.

I think there is value in doing a review (specially with junior team members) on their 'design intent' early on, as soon as a prototype/skeleton is up. Proposing a change then is met with a lot less friction.

  • Chris_Newton  2 years ago

    Many years ago, I was on a working group looking at different ways to improve quality within a fairly large software development company. One of the most useful ideas the consultants brought to the table was that we should have “technical reviews” at key stages in the development process. The arguments back then were much the same as they are today: identify potential problems the original developer missed, share interesting ideas in time to consider and use them, making changes earlier is usually easier and cheaper, etc.

    Today code reviews have become common practice, which is definitely a change for the better, but I rarely see assets like specs or design work reviewed with the same consistency and attention to detail. If anything, the trend has been to minimise formal requirements capture and to reduce or eliminate any kind of up-front design work, as if there is some kind of bizarre dichotomy where the only amounts of time you can invest in these activities before starting to write code are measured in either months or minutes. I believe this is a mistake that often leads to avoidable wasted work and tech debt.

ludston  2 years ago

But don't forget that there are two reasons why something will not be obvious: It may not be obvious because the meaning is obfuscated, or it may not be obvious because it relies on the reader understanding certain ideas, concepts or technologies.

  • daveslash  2 years ago

    This. I once had a code review with a new hire who couldn't understand a for loop in C#. Something almost as simple as the snippet below. Their resume showed a B.S. in CompSci from a CalState school. But they professed "I never understood loops".

      for(int i = 0; i < 100; i++)
  {
    newList[i] = sourceArray[i];
  }

    I've also been in working environments where management has insisted that the code reviews be moderated by someone who was a mechanical engineer with no code/software training, background, or experience. I didn't particularly enjoy those....

bryanrasmussen  2 years ago

>One takeaway I had: code reviews matter. If your code is undergoing review and a reviewer tells you that something is not obvious, don’t argue with them; if a reader thinks it’s not obvious, then it’s not obvious.

ok, what about if you have code and you think this is not obvious because edge case for browser X version Y therefore I will leave a long comment specifying why it is the way it is and when and under what conditions in the future it should be removed - but the reviewer thinks it is obvious and please remove the comment.

As a general rule reviewers concerns should be addressed, but I have had some experiences in which what the reviewer wanted made the code worse, or even would possibly introduce hard to find bugs.

laurent_du  2 years ago

If I ask my kids or my girlfriend to review my code, nothing will be obvious to them. Doesn't mean that my code is the problem. The idea that the reviewer is always right makes no sense.

  • glun  2 years ago

    Your family is not the intended audience for the code. If the reviewer isn’t either, then they have no business reviewing your code.

    • marcosdumay  2 years ago

      I can agree with that. But the intended audience for your code isn't as rigid and formalized a thing as review processes make the reviewers identities.

      That actually means that review processes are usually wrong. But then, people experiences are about the wrong process, and that's what they react on.

agumonkey  2 years ago

good point but it's culture dependent

i had people telling me

   d = {i:str(i) for i in range(10)}

was too hard to read, preferring

    d = {}
    for i in range(10):
        d[i] = str(i)

  • conkeisterdoor  2 years ago

    I also strongly prefer the dictionary comprehension and find it much more readable - for whatever my 2c is worth.

    • agumonkey  2 years ago

      to me it's even beyond readability, it's a close oneliner scope.. less opportunity to insert some weird statement / bug in it

      but anyway, to some senior engineer, the dict comprehension is a chore