In this lecture there is some code duplication, where the health is checked to see if it is zero and then calling Die() if it is, both in the TakeDamage and RestoreState methods. Wouldn’t it be more proper to extract this code into a method called CheckForDeath or something so that if we want to change something about the death procedure we only need to change it in one place, and avoids creating inconsistent behaviour if we change one method but not the other?
It can go either route. I’d say it would be handy to make a separate method to check the health, so I’d encourage that if you feel that it’s better for your code.
Sam has mentioned (more than once iirc) that we only care about code duplication when we start duplicating a third time.
Also, and this is something I myself are very guilty of, we should be wary of creating code that may or may not be used in the future. Don’t write code because you think it may be used in the future (Unless you are creating some sort of API). If we need to change something about the death procedure, then we can worry about that and refactor. We shouldn’t be writing code because we think that maybe, by some off chance, we may want to change something about the death procedure.
Regardless, the ‘death procedure’ is already in a separate method: Die(). The only thing that’s not in there is checking whether or not it should execute. All a separate method does is take away the control from the consumer, which can no longer decide if it wants to execute Die() or not. It’s a toss-up between protecting the consumer against calling Die() prematurely, or letting it choose to call it or not.
That’s a good point. I’m not 100% convinced of why we need to wait until a third duplication though. I guess it depends on how much code is being duplicated. Everything is a tradeoff.
One way around the issue would be to not have a Die function at all, but a CheckForDeath function, which does the checking for health being zero and dying together.
I’m somebody who is absolutely guilty of “overengineering”… As soon as I see a pattern developing, I refactor it, and I often think three moves ahead about “oh, wait, I’ll probably want to make an API out of this”…
The problem with this approach (and I speak from experience), is that when you start overengineering and refactoring too early, you tend to get distracted from the actual task you’re trying to accomplish. You can also inadvertently introduce errors in code that was working correctly to begin with. Definitely not saying “don’t refactor” (I would be quite the hypocrite, there). Just saying be mindful and consider what you’re gaining by refactoring. Consider a //TODO: for now and get back to refactoring when you’ve gotten things a bit further along.
2 Likes
That’s a good point. I think I’ve got into that overengineering trap myself more than a few times. So many things are a balancing act in this.