4 ms·

Ask HN: Code review vs. Unit test

16 points by cryptozeus 9y ago

Hi, Within our company there is an internal debat going on about what process should we focus on first to improve code quality. Unit testing frameworks or code review tools ? IMO code reviews has become just a formality where developers just check for syntactical issues instead of overall design. This is mainly due to tight deadline where everyone is trying to finish their work. Has anyone tried putting this process in large team ? What are the before and after effcts ?

yen223 9y ago
We do both. Unit tests can help with the review process, by making it clear the intention behind certain aspects of code changes. Code reviews and unit tests are only as effective as the developers who participate though, can't escape that.
llccbb 9y ago
What are the actual problems that your company is encountering? Bugs? Spaghetti? Unmaintainability?
wolco 9y ago
I think unit testing and automated tests are the way to go with pull requests that require two developers approval. Two developer number is important on the reviews to keep everyone honest and to keep everyone interested.
jamestimmins 9y ago
Sounds like it's worth taking a step back and looking at your processes from a high level. If there's pressure to push code more quickly than is possible while maintaining effective code reviews, then you may want to consider where that pressure is coming from. If velocity is causing quality control issues, then velocity is likely too fast. Not to mention that limiting quality checks will reduce velocity in the long term. Once you have both product and engineering (or whatever forces are at play) on the same page about reasonable expectations, I'd recommend using both. Code review and automated testing both serve distinct yet important roles.
- tdbgamer 9y ago
  Yeah, if code reviews are just syntax checks, then they're doing it wrong and no tool will ever fix their problem. However if they have more junior devs and want to prevent major design mistakes before they happen, pair programming is a thing.
toast0 9y ago
If you're in a tight deadline environment, I'm guessing people aren't going to spend quality time on unit testing or code reviews. It's quick and easy to write terrible unit tests that are counter productive; it's also pretty easy to just stamp all the code reviews without taking a real look. Not every piece of code needs to be high quality. Instead, you should indentify the parts of your system that need higher quality and spend real time reviewing code and writing appropriate tests (not just unit tests, also do performance tests and integration tests of key components). Especially in an environment with tight deadline and changing requirements, focusing on key areas will save a lot of effort.
jwilliams 9y ago
In reality, you need both. However, unit tests are code. If you're writing poor quality code, then you risk writing poor quality unit tests. Brittle, overly specific unit tests can be a major burden. Even just mediocre unit tests can weigh down a project. Code reviews have an outsized impact in the organizations I've worked in. More people understand more of the code. Quality goes up not only because the bad code is spotted, but also the reviewers can learn good practices too. You also get better at reading code. This is a fundamental, critical skill. For this reason, I'm a fan of having developers of all skill levels be reviewers. However, code reviews are a skill and take an investment for people to learn how to be effective. So if you're really, really having to pick, do code reviews. Use the code reviews to drive sensible, smart adoption of unit testing.
shados 9y ago
I'd spend time trying to fix the environment where deadlines are so silly that you can't do proper code reviews.
bzalasky 9y ago
Honestly, you need both. Here's a comment on code review I wrote year ago or so that seems relevant: Code reviews shouldn't dwell on style issues that can be handled by linting. To the extent that you can automate some of the review process, and prevent sloppy commits from being merged in by having them fail CI, you'll eliminate part of the problem. Another thing that helps is reducing the semantic distance between what a commit message says a commit does, and what it actually does. This is pretty easy to enforce relative to more subjective aspects of coding. The last thing that can make a big difference is not getting hung up on wanting to rewrite a commit as you would have written it, but focusing on improving understanding and clarity for whoever will inherit the code next. If there are serious issues with the architecture and patterns the code is using, CR can be too late in the process to address them effectively (depends on the scale involved). I'd try to collaborate with other engineers earlier in the process to help set them on the right path [1]. As for unit tests, make sure to isolate the unit (generally a class or function), and test its interface. Tests that are coupled to the implementation aren't as useful. Investing in writing unit tests now will increase your velocity in the long run. You'll be able to take on more technical debt in the short term with better terms, so to speak, since you'll have a robust test suite to refactor against. Obviously, this doesn't mean you should write bad code, but this will give you the option of considering performance and reusability tradeoffs if you need to hustle a feature out the door. [1]: https://news.ycombinator.com/item?id=11257835 https://news.ycombinator.com/item?id=11257835
twobyfour 9y ago
Code reviews are not a good substitute for unit tests; and it sounds like what you're describing your team as doing is more like linting, which you should use an automated tool for. The purpose of unit tests is to catch regressions. They can also be an excellent tool to speed development in real time by using automated tests to confirm the code does what you want it to, instead of repeated click-through testing, which is often slower. A habit of unit testing can (but doesn't always) encourage developers to adjust their mindset about writing code and to get into better habits. For instance, feature-oriented spaghetti code is hard to test. API-oriented code is easier to test, easier to maintain, and easier to extend. Developers thinking ahead about testing, or writing tests and application code in parallel, are also more likely to use patterns like dependency injection. Code review is a terrible way to catch bugs, but it can be an excellent way to share knowledge. Both knowledge of the craft, shared from seniors to juniors; and domain knowledge / institutional memory of business practices, ins and outs of existing code, and the like. It's also a great opportunity to review design decisions and teach best practices in that regard. You mention code review failing due to tight deadlines and everyone "just trying to finish their work". I see two potential issues here. The first is the obvious one that it sounds like your team is a little overworked/understaffed. There are all sorts of reasons (that I'm not going to go into here) why it's bad for a team to have zero slack time. But not having time to step back and assess the outcome of the work is - as you've discovered - a major one. The second issue is that it sounds as if your team might be a little short on cohesion. One way this might manifest is that they don't trust one another to give or receive feedback in an empathetic way, so they're afraid of requesting or providing in-depth feedback. Another is that they don't feel a personal obligation to one another to make time to provide the sort of in-depth feedback that anyone should desire if they want to continue improving at their craft (and which is one of the primary benefits of working on a team instead of solo). A useful code review on a 1000-line changeset doesn't have to take more than 15 minutes. I don't know exactly what sorts of code quality issues your organization has been encountering, but I can add an anecdote. My team has introduced both code review and unit testing in the past 18 months. Code review was adopted hesitantly at first, but then with enthusiasm. I wouldn't say it's had much impact on typical code quality or caught many bugs, but it has improved communication and caught a few architectural mistakes that might have been painful to deal with if they'd made it to production. Unit testing didn't really catch on until we implemented continuous integration to make sure the tests ran - and passed - after every change. In fact, getting the first green build took a week of fixing tests that hadn't been run or updated in months. Since then we've also added a Slack bot to shame anyone who breaks the trunk build, and our test suite has expanded from roughly 25 tests to about 600. And our tests have uncovered or prevented more regressions than I can count. Some team members have taken to unit testing with more enthusiasm than others, despite a directive that all new development must have test coverage. One team member uses it proactively, even to reproduce bugs before fixing them; another uses it only for green-field development; another I have to pull teeth to convince to add tests (even though he concedes that they're enormously useful for catching regressions and verifying that dependency upgrades are safe, and even though his design decisions have leveled multiple levels up since we started testing); another built an entire back-end feature by running his code only in the test suite because he didn't have a UI to run it in and the setup was too complex to run comfortably in the REPL.
quickthrower2 9y ago
If you have to pick one, pick code reviews. Without them you really can end up with shitty code and not know about it for months or years and have it bite you. Unit tests without code reviews can be useless if they are testing the wrong things or not deep enough.
psyc 9y ago
My intuition based on 20 years of working on large-ish projects, is that code reviews are always helpful, and mandatory unit tests waste a lot of time and effort, at least for statically typed code. I don't have much experience with dynamic languages. This doesn't mean don't write any unit tests, but to use discretion instead of enforcing coverage. So, it's perfectly reasonable for a code reviewer to say, "a unit test could be really helpful here."
hellwd 9y ago
I have tried a lot of times to enforce the code reviews because I have seen lot of bullshit being checked in to the source control but if you don't have support from other team members then even having the code review is waste of time. Code review should come from the gut feeling, you should be always self critical and let your code being evaluated by other people who care about software development in general and not by the people who just want to make their job done and to have something to mention during the stand up meeting. So depending on what kind of people you are surrounded with, you should decide is it worth to do it or not. Code review is not a task or ticket, it should be more natural in all teams. Like it's normal to take a shower or brush your teeth, it's also normal to do code reviews and keep the code clean and good. But recently most of the teams are just skipping everything to make managers and owners happy. And they are always happy with beta, preview or even prototype - so sad. Unit tests are something else. Good, clean and testable code starts from good and clean requirements that are easily understandable. From there you are able to write clear unit tests and imagine the whole story. If your manager or someone who is defining what needs to be done doesn't have the clear vision or plan then you have a good chance that your code will be bad - then unit tests are implicitly bad.
jbreckmckye 9y ago
Code reviews come first, because you can't write good unit tests without them. And poor tests can make your life horrible.

16 points by cryptozeus 9y ago

yen223 9y ago

We do both. Unit tests can help with the review process, by making it clear the intention behind certain aspects of code changes. Code reviews and unit tests are only as effective as the developers who participate though, can't escape that.

llccbb 9y ago

What are the actual problems that your company is encountering? Bugs? Spaghetti? Unmaintainability?

wolco 9y ago

I think unit testing and automated tests are the way to go with pull requests that require two developers approval. Two developer number is important on the reviews to keep everyone honest and to keep everyone interested.

jamestimmins 9y ago

Sounds like it's worth taking a step back and looking at your processes from a high level. If there's pressure to push code more quickly than is possible while maintaining effective code reviews, then you may want to consider where that pressure is coming from. If velocity is causing quality control issues, then velocity is likely too fast. Not to mention that limiting quality checks will reduce velocity in the long term. Once you have both product and engineering (or whatever forces are at play) on the same page about reasonable expectations, I'd recommend using both. Code review and automated testing both serve distinct yet important roles.

tdbgamer 9y ago
Yeah, if code reviews are just syntax checks, then they're doing it wrong and no tool will ever fix their problem. However if they have more junior devs and want to prevent major design mistakes before they happen, pair programming is a thing.

toast0 9y ago

If you're in a tight deadline environment, I'm guessing people aren't going to spend quality time on unit testing or code reviews. It's quick and easy to write terrible unit tests that are counter productive; it's also pretty easy to just stamp all the code reviews without taking a real look. Not every piece of code needs to be high quality. Instead, you should indentify the parts of your system that need higher quality and spend real time reviewing code and writing appropriate tests (not just unit tests, also do performance tests and integration tests of key components). Especially in an environment with tight deadline and changing requirements, focusing on key areas will save a lot of effort.

jwilliams 9y ago

In reality, you need both. However, unit tests are code. If you're writing poor quality code, then you risk writing poor quality unit tests. Brittle, overly specific unit tests can be a major burden. Even just mediocre unit tests can weigh down a project. Code reviews have an outsized impact in the organizations I've worked in. More people understand more of the code. Quality goes up not only because the bad code is spotted, but also the reviewers can learn good practices too. You also get better at reading code. This is a fundamental, critical skill. For this reason, I'm a fan of having developers of all skill levels be reviewers. However, code reviews are a skill and take an investment for people to learn how to be effective. So if you're really, really having to pick, do code reviews. Use the code reviews to drive sensible, smart adoption of unit testing.

shados 9y ago

I'd spend time trying to fix the environment where deadlines are so silly that you can't do proper code reviews.

bzalasky 9y ago

Honestly, you need both. Here's a comment on code review I wrote year ago or so that seems relevant: Code reviews shouldn't dwell on style issues that can be handled by linting. To the extent that you can automate some of the review process, and prevent sloppy commits from being merged in by having them fail CI, you'll eliminate part of the problem. Another thing that helps is reducing the semantic distance between what a commit message says a commit does, and what it actually does. This is pretty easy to enforce relative to more subjective aspects of coding. The last thing that can make a big difference is not getting hung up on wanting to rewrite a commit as you would have written it, but focusing on improving understanding and clarity for whoever will inherit the code next. If there are serious issues with the architecture and patterns the code is using, CR can be too late in the process to address them effectively (depends on the scale involved). I'd try to collaborate with other engineers earlier in the process to help set them on the right path [1]. As for unit tests, make sure to isolate the unit (generally a class or function), and test its interface. Tests that are coupled to the implementation aren't as useful. Investing in writing unit tests now will increase your velocity in the long run. You'll be able to take on more technical debt in the short term with better terms, so to speak, since you'll have a robust test suite to refactor against. Obviously, this doesn't mean you should write bad code, but this will give you the option of considering performance and reusability tradeoffs if you need to hustle a feature out the door. [1]: https://news.ycombinator.com/item?id=11257835 https://news.ycombinator.com/item?id=11257835

twobyfour 9y ago

Code reviews are not a good substitute for unit tests; and it sounds like what you're describing your team as doing is more like linting, which you should use an automated tool for. The purpose of unit tests is to catch regressions. They can also be an excellent tool to speed development in real time by using automated tests to confirm the code does what you want it to, instead of repeated click-through testing, which is often slower. A habit of unit testing can (but doesn't always) encourage developers to adjust their mindset about writing code and to get into better habits. For instance, feature-oriented spaghetti code is hard to test. API-oriented code is easier to test, easier to maintain, and easier to extend. Developers thinking ahead about testing, or writing tests and application code in parallel, are also more likely to use patterns like dependency injection. Code review is a terrible way to catch bugs, but it can be an excellent way to share knowledge. Both knowledge of the craft, shared from seniors to juniors; and domain knowledge / institutional memory of business practices, ins and outs of existing code, and the like. It's also a great opportunity to review design decisions and teach best practices in that regard. You mention code review failing due to tight deadlines and everyone "just trying to finish their work". I see two potential issues here. The first is the obvious one that it sounds like your team is a little overworked/understaffed. There are all sorts of reasons (that I'm not going to go into here) why it's bad for a team to have zero slack time. But not having time to step back and assess the outcome of the work is - as you've discovered - a major one. The second issue is that it sounds as if your team might be a little short on cohesion. One way this might manifest is that they don't trust one another to give or receive feedback in an empathetic way, so they're afraid of requesting or providing in-depth feedback. Another is that they don't feel a personal obligation to one another to make time to provide the sort of in-depth feedback that anyone should desire if they want to continue improving at their craft (and which is one of the primary benefits of working on a team instead of solo). A useful code review on a 1000-line changeset doesn't have to take more than 15 minutes. I don't know exactly what sorts of code quality issues your organization has been encountering, but I can add an anecdote. My team has introduced both code review and unit testing in the past 18 months. Code review was adopted hesitantly at first, but then with enthusiasm. I wouldn't say it's had much impact on typical code quality or caught many bugs, but it has improved communication and caught a few architectural mistakes that might have been painful to deal with if they'd made it to production. Unit testing didn't really catch on until we implemented continuous integration to make sure the tests ran - and passed - after every change. In fact, getting the first green build took a week of fixing tests that hadn't been run or updated in months. Since then we've also added a Slack bot to shame anyone who breaks the trunk build, and our test suite has expanded from roughly 25 tests to about 600. And our tests have uncovered or prevented more regressions than I can count. Some team members have taken to unit testing with more enthusiasm than others, despite a directive that all new development must have test coverage. One team member uses it proactively, even to reproduce bugs before fixing them; another uses it only for green-field development; another I have to pull teeth to convince to add tests (even though he concedes that they're enormously useful for catching regressions and verifying that dependency upgrades are safe, and even though his design decisions have leveled multiple levels up since we started testing); another built an entire back-end feature by running his code only in the test suite because he didn't have a UI to run it in and the setup was too complex to run comfortably in the REPL.

quickthrower2 9y ago

If you have to pick one, pick code reviews. Without them you really can end up with shitty code and not know about it for months or years and have it bite you. Unit tests without code reviews can be useless if they are testing the wrong things or not deep enough.

psyc 9y ago

My intuition based on 20 years of working on large-ish projects, is that code reviews are always helpful, and mandatory unit tests waste a lot of time and effort, at least for statically typed code. I don't have much experience with dynamic languages. This doesn't mean don't write any unit tests, but to use discretion instead of enforcing coverage. So, it's perfectly reasonable for a code reviewer to say, "a unit test could be really helpful here."

hellwd 9y ago

I have tried a lot of times to enforce the code reviews because I have seen lot of bullshit being checked in to the source control but if you don't have support from other team members then even having the code review is waste of time. Code review should come from the gut feeling, you should be always self critical and let your code being evaluated by other people who care about software development in general and not by the people who just want to make their job done and to have something to mention during the stand up meeting. So depending on what kind of people you are surrounded with, you should decide is it worth to do it or not. Code review is not a task or ticket, it should be more natural in all teams. Like it's normal to take a shower or brush your teeth, it's also normal to do code reviews and keep the code clean and good. But recently most of the teams are just skipping everything to make managers and owners happy. And they are always happy with beta, preview or even prototype - so sad. Unit tests are something else. Good, clean and testable code starts from good and clean requirements that are easily understandable. From there you are able to write clear unit tests and imagine the whole story. If your manager or someone who is defining what needs to be done doesn't have the clear vision or plan then you have a good chance that your code will be bad - then unit tests are implicitly bad.

jbreckmckye 9y ago

Code reviews come first, because you can't write good unit tests without them. And poor tests can make your life horrible.