Hacker News Reader

Comment by alkonaut | Hacker News Reader

This is a cautionary tale about a) commenting non-obvious code and b) adding tests. Someone had to think hard about how to solve this problem. Everyone coming after shouldn’t have to. I must always be able to do drive-by refractoring with just seconds of thought. If tests are green and quick manual validation doesn’t show anything broken - then I’ll assume the refactoring was ok.

The job isn’t to make something work. It’s to make it work AND be resilient to a bad refactoring. If you didn’t properly protect your implementation with tests and comments you didn’t implement it at all.

View on HN

To the downvoters: why? Genuinely curious.

I have not down voted it myself but there is the assumption that unit tests will always catch bad refactorings, that human error doesn't apply to writing unit test, that every developer is perfectly conscientious, competent, is not under tight deadlines etc., that your legacy code base has comprehensive unit tests..

The author here: I agree with @Gupie, having unit tests ensures that you have correctly refactored the code and have not introduced new bugs. Checking the unit tests can also help you better understand what problems the code is solving and the edge cases you may be missing.

Normally, if unit tests were missing, I would add them before starting refactoring. In this case, to be honest, I'm not sure how I could have written a unit test to catch the issue. An E2E test is the only one that comes to mind. Please let me know if I am missing an easy way to test for a 404 image when offline. Thank you!

The danger is in the combination of all 3: non-obvious assumption (code should work offline), lack of tests, lack of documentation of the assumption.

Flip ANY of the three, and there is no problem. Had I been the original author I would have gone for the comment, it’s clearly the simplest one in this case.

Try to actually design the test that would have caught this. There is no way in this universe that one can blindly rely on tests when refactoring.

Code that is obvious is better. That’s also difficult here (You’d for example make a function call of image_that_should_work_offline(..)” that simply returns its argument, but that’s perhaps a bit contrived). So the second best is simply a comment.

// Image inlined because error may be due to dropped connection

You don’t have to have tests to prevent bad refactoring. 100% test coverage isn’t realistic for most code bases (or isn’t even a net positive in many cases).

self-documenting code, or tests, or comments would have saved the situation. You don’t need to design a test in this case. A one line comment that takes five seconds to write is enough.

Reply on news.ycombinator.com