Hacker News Reader

These are all good tips, but I want to stress the importance of (6). Much of what the original post is talking about can be improved by adopting the habit of making small, incremental changes. As the author, you no longer have to wait "in the order of weeks" for the feedback and a potential rebase should pose little risk. As the reviewer, you don't have as much mental burden and most reviews can be done "in-between" bigger tasks, e.g. in the 15 minutes before a meeting or before lunch, etc.

As for 1) I highly recommend to automate that away as much as possible. A strict syntax guideline and a toolchain that enforces it will go a long way.

Conceptually, I completely agree with (6). Practically, though, I found it really hard to do in some situations.

Specifically, when doing something where the solution is well known for the beginning, creating PRs which are small and easy to understand is easy.

When instead I have to solve difficult problems for which I don't already know the solution beforehand, and which require writing quite a bit of complex code - eg multiple classes that work with each other, and which maybe work with some complex external system I/the team doesn't already know well - I find it difficult to split the PR into smaller chunks, because I "evolve" the separate classes together, while also doing exploratory testing.

"Production ready" code comes later in the process, and at that point I usually have quite a bit of code. I noticed that people don't like reviewing the non-production-ready code, but what's the alternative? In theory, after I found out what the solution is, I could go back and rewrite it better from the beginning, but that would just require too much time.

Didn't find a good answer yet.

The answer to your problem of "large commit" is this:

1) Create "research prototype" of your solution. But do not commit it or commit it into separate branch (not master).

2) Create a plan how to split that code into smaller steps. Separate refactorings that will support your new feature - from your actual feature (that changes functionality).

3) Commit refactorings separately from your main feature commit. Keep every refactoring commit as small as possible).

4) At the end commit your functionality changing feature. If possible, split it into multiple commits too.

That should help with code review.

Remember, your team will have to code review your code multiple times:

1) When you are making your commits.

2) When your code reviewer reviews it.

3) In the future, when you or other team members maintain your code and try to understand why you created code that way.

So - optimize your commit to reduce code review time (even if it increases initial code writing time).

Thank you for your answer, but I don't think that would have been feasible - our employer called me exactly because the team was very slow, and adding so many additional steps would make me very slow too.

I'm now changing contract, hopefully with the new team it will be possible to find a better compromise. For example, I offered to walk the reviewer through my code, which I think would have helped a lot, but this was refused...

> additional steps would make me very slow too

Your real choices are: "fast and buggy" vs "slow and correct".

What is your preference?

> I offered to walk the reviewer through my code

Walking reviewer through your code is a good exercise. Code review of complex change should be done in interactive session (code reviewer + coder). There is so much to learn for both sides in such session.

https://dennisgorelik.dreamwidth.org/161605.html

> Your real choices are: "fast and buggy" vs "slow and correct".

It's not a binary choice, it's a dial. And anyway, after more than 30 years of programming experience, I can tell you that I can write mostly correct code by applying diligently the pyramid of tests. In my experience, code reviews catch very few bugs that weren't revealed by testing, if any.

Much more than correctness, I find them useful to verify and improve the readability and maintainability of code - which are important goals, but as always the cost needs to be measured against time and cost constraints. Depending on the project, being twice as slow to accomodate the reviewer can or can't make sense. What I'm looking for is more of a 80/20 solution: can I be 20% slower, and still get 80% of the benefits of code reviews?

I think these points are very salient. I'd suggest adding another one: create a checklist for code review that is relevant to the issues your team faces. The checklist makes it much easier for you as a reviewer to perform evenly.

Great idea, thanks!

> 3) If I'm in a hurry and the author is available I grab them and do an over the shoulder review. You might be surprised how much time this saves.

One concern here is you may ask questions whose answers are then given to you, and you alone. If it's likely someone else in the future will have the same questions, the "answers" should be in the form of improved code.

> 4) Don't struggle with difficult sections of other people's code unless you have a vested interest in remembering this specific section of code. Ask about unclear areas.

I'm not clear on the distinction between "difficult sections" and "unclear areas"? Either way, one of the outcomes of the review should be to make life easier for the folks that follow us.