Hacker News new | ask | show | jobs
by lisivka 3505 days ago
Peer review is just request for short review. "Shortly look at this new piece of code, before I will commit it into our repository, and say your opinion." That's all.

If you will ask me to review piece of your code, written like that, I will immediately ask to write test cases, because team will not be able to improve code base without test coverage, and to add meaningful documentation, so we will spend our time on work instead of solving mental puzzles. In team, your freedom ends where freedom of other members begin.

If you are thinking that you are extremely good developer, then finish your projects, with your team, in fraction of time, comparing to other teams, otherwise you are extremely good programmer, but not an extremely good developer.

> Please do let me know about bugs!

There is no obvious bugs in C code. There are few tiny problems in bash scripts (e.g. error messages are not printed to STDERR and error messages are not helpful). There are lot of problems with style and documentation, which will affect team velocity and maintenability. Peer review is not a bug review. I just need to ensure myself that new code will not create problems for us when committed, and the easier way to do that in less than 10 minutes is to look at test cases, so write test cases and documentation first. When they will be OK, I may say that your code is OK. ;-)

2 comments

As has already been mentioned, there are industries where the code simply must have no bugs and in those code reviews will be different. Most critical components may be written by a very small team perhaps even a single person, as multiple people working on the same thing will generally result in more bugs.

Again I didn't say tests are a bad idea. In industry projects they are usually a very good idea. My point was that people should be able to write working bug-less code without using tests. When a test catches a real bug, it is to be understood as a failure of the developer(s).

I saw so many bugs, so I switched from "code is working" by default to "code is broken" by default unless it proven that code is working correctly at target environment with required constraints.

Moreover, test cases are saving development time massively, so why waste time for manual tests and debugging sessions, when I can use computer to do that instead of me? When practiced regularly, test cases are written in tens of seconds. For example, if I will work on a calculator, I will write something like assertEquals(calculate("2+2"), 4, "Result of arithmetic addition is wrong."); Then I will work on calculator until this test will pass. Only then I may start to write unit tests. It's easy and fast, so why not?

> If you are thinking that you are extremely good developer, then finish your projects, with your team, in fraction of time, comparing to other teams, otherwise you are extremely good programmer, but not an extremely good developer.

Actually, I do! My work code does have more comments and tests (for my hobby projects, I do less of that and instead write other code, getting more done faster).

> There is no obvious bugs in C code. Indeed you still haven't pointed out a single bug, so you have not yet invalidated my assertion that I can write (mostly) bugless code :)

I pointed to one bug: error messages are not redirected to stderr in shell scripts. It's small bug, because it will affect development only, but I found it. I need to find just one bug to prove my point, isn't?
It's not a bug, you can't call just any deviation from the ideal a bug :)
If you redirect stdin to /dev/null, to see error messages, you will not see these error messages. If you try to grep them by "error" keyword, you will not see them either. So you will have two options — read everything using your own eyes OR fix bug in the script. So this is the bug, which causes significant time losses for OPS team pretty often (once 2-3 months someone is bitten by that).