I'm sure you spend a lot of time thinking about how to write better software just like I do. And if you've dipped your toes into the waters of unit...
For further actions, you may consider blocking this person and/or reporting abuse
Great post. I've seen the cycle of misery a thousand times:
Tests support many development goals. Verifying that the code works correctly now is just one. It's arguably more important to have tests that verify that the code continues to work as it changes over time.
I love it! The "cycle of misery" is the perfect term to describe it.
Another error is when you send an array with count(array) <= 1, because the count(arr) is NOT the first check, as it should be. Depending on the language also NULL can do the same error. (null[0] or null[1] would not work).
Good catch. That actually can't happen because if the way the data is generated but I agree the size check should be before the equality check. I've updated the production code to match.
Thanks for your comment.
Yes, I am completely with you on this. Manual code reviews must not be skipped.
A well-known technique is also en.wikipedia.org/wiki/QuickCheck (you may find it under different names depending on the language).
The goal is to automatically generate random values to pass in a function and see how it reacts.
Yes. Quickcheck is an implementation of property-based testing. We do property-based testing mostly for reversible operations like encode-decode functionality when we want to make sure the data is preserved. But that's really a niche for us. The base of our testing pyramid is unit tests.
I'm not familiar with the language, but it looks kinda strange that validation function has
voidtype. Wouldn't it be better to returntrueorfalseinstead of throwing exceptions?Is there any reason for passing an array of values instead of 2 params?
Yes. That's what I was trying to discuss towards the end of the post. It is confusing.
Passing an array instead of two params was the author's choice. It meets our coding standard and it makes a fair bit of sense in the larger context of where that data comes from. Two params would have been perfectly fine in this context too.
Great article!
I agree that nothing is too simple to test.
I've been writing unit tests for a while now and I can say that you need to unit tests every method you have. Even the simplest ones can create bugs sometimes...
Thanks. Carry on...
Great post and very interesting discussion/code review.
I've seen an even eviler variation of the example you've shown us. Someone has called a validation method and stored its result in a boolean variable. After the iteration result stored in the method, the variable has been returned to a caller. It could end with false positive or false negative result. The result depends only on the last partial validation in the foreach loop, because of the overwriting the method variable in each iteration.
Luckily we've found the issue during code review proces.
...thereby proving that WTFs/minute is still the only true measure of code quality.
(I've seen something like your example as well.)
You should watch Uncle Bob (Martin) on Unit Testing. He doesn't believe anything is too small to test. In fact you should test that you can create an object. I tend to agree with him.
Also 15 lines of code in a method is a lot. Anything above 10 in my opinion is a lot if you're trying to follow single responsibility.
Yes. I've watched uncle Bob's stuff on unit testing. TDD is a little different than getting tests around existing code. But, yes, I agree. I'd never fail a PR because the unit tests were testing things that were simple.
I'm actually with Phil Koopman on the issue of method complexity/size. There's a sweet spot and I think it's bigger than uncle Bob recommends for my projects. That doesn't mean I like huge methods. But when I've chopped my code up into methods like uncle Bob recommends, I end up with really deep call chains and it becomes harder to understand what's happening.
I don't know the language. What happens if you compare the first category to the second category but there's only one category? Will it get to the second check which checks to see if there are two?
Good question! I actually had to run some code to make sure it behaves as I expected.
It will get to the check for the number of elements in the collection unless the first element in the array is null. If that happens the first "if" returns true and you'll get an exception, which is what we want but the error message will be deceptive. That's a real edge case though.
I was hesitant to mention it because I don't details of the language. I was just curious because I didn't know if you could compare the first and second elements without an exception if there were fewer than two elements. In .NET the check itself would throw an exception, so I'd check the number of elements first.
No worries. We're just talking. Bring up anything you like.
I swapped the if statements this morning in the actual code (not updated in the blog post). So the size check is now before the equality check.
If you turn up the warnings enough in PHP you'll get a notice about accessing a nonexistent array element but it won't throw an exception on its own.
The thing that immediately caught my eye was the way the variable naming changed from using "ID" to "Id" half way through, which made me expect it to be an error around that.
Good eye. It's so interesting to hear all these comments and suggested improvements for what amounts to a pretty trivial and boring piece of code. My thanks to everybody who commented.
I'm curious how many of you run your code reviews at this level of detail? Are the things you are mentioning typical things you'd pickup and mention in a regular code review at your job? Or is this a special level of feedback because of the circumstances?
I caught the naming issue too and fixed it in the production code before I released the blog post.
Great post!
Thanks, Ben. I enjoyed writing it.