Can you give a more specific example please? I don't see anything wrong with this, for instance,
| file |
file := self loadPaymentFile: self sample.
self assert: file validPayments size = 3.
self assert: file invalidPayments size = 1.
Well, I would probably change the file to a stream, but that's a different topic and discussion (I generally don't like to access outside resources in my test; they only cause problems later). Back to the conversation, the last two lines are what I don't like. The reason is that say another developer changes any of the code that gets called during the loadPaymentFile: and they run all tests. And let's say it's in common code and this test fails. Let's also say that we get back 2 valid payments and 2 invalid payments.
The developer is going to have no clue which one became invalid and what made it valid in the first place. You could put in comments explaining what the 3 valid payments and 1 invalid payment were to help. or you could break out and have several asserts for each of the cases. Otherwise, I have to revert code and rerun this test to see which payment is now incorrect. And then, put my code in and see where my code is incorrect. The above code makes the original intent hard to understand. Why were only 3 valid? Why was only 1 invalid and what made it invalid?
Really the above test is not incorrect per se, it's just not very thoughtful of the other developer's time. A couple of extra minutes of coding time would save the other developer a couple of hours. It's about coding and feeling sorry for the poor person that has to come behind you and figure out what you did. I try to always code with the maintainer in mind. Most times, the maintainer happens to be me and I'm always thankful I took the extra time to help myself.
No comments:
Post a Comment