Forums » Help » "Best Practice" says a function should perform only a single task, so...

I keep hearing how a properly written function should have a single function.

Take this naughty function for example which is called each time a bullet entity collides with the player entity:

takeDamage: function(damage) {
    if(this.sounds.impact) {
        this.sounds.impact.play();
    }
    if(this.god) {
        return;
    }
    this.lastHitTimer.reset();
    if(this.shields > 0 && this.shields - damage >= 0) {
        // The shield can take ALL of the blow.
        this.shields -= damage;
    } else if(this.shields > 0 && this.shields - damage < 0) {
        // The shield can take SOME of the blow.
        var healthDamage = damage - this.shields;
        this.shields = 0;
        this.health = Math.max(this.health - healthDamage, 0);
    } else if (this.shields <= 0) {
        // The shield can take NONE of the blow.
        this.health = Math.max(this.health - damage, 0);
    }
    // Handle possible 'dead' scenario.
    if(this.health <= 0) this.noMoreHealth(); 
}

According to the one task per function principal. This function should be rewritten like this:

takeDamage: function(damage) {
    if(this.shields > 0 && this.shields - damage >= 0) {
        // The shield can take ALL of the blow.
        this.shields -= damage;
    } else if(this.shields > 0 && this.shields - damage < 0) {
        // The shield can take SOME of the blow.
        var healthDamage = damage - this.shields;
        this.shields = 0;
        this.health = Math.max(this.health - healthDamage, 0);
    } else if (this.shields <= 0) {
        // The shield can take NONE of the blow.
        this.health = Math.max(this.health - damage, 0);
    }
}

Yes, this looks easier to read and manage.

But now I'm left with the question, where do I put all the rest of the code I took out?

Before upon collision with the player, a bullet entity would call player.takeDamage(100); and that was it.

So does this mean that the bullet entity is supposed to call the following now instead?:

player.playHitSound();
if(!player.god) {
    player.takeDamage();
    player.lastHitTimer.reset();
    player.checkIfDead();
}

It seems like at the very least this is violating some object oriented programming principals. I mean, should the bullet entity really be aware that player has variables such as god, and lastHitTimer? Probably not.

I've probably structured things fundamentally wrong, but I'm not sure how I should have come at this any differently.

So, have you guys been having much success with "short concise single purpose functions", and do you see an elegant way to improve upon this example?

A well written function should have a single responsibility. This is of course more of a guideline than anything, so don't think of it like a hard and fast rule.

Your last method looks clean, but should be part of the player's takeDamage function, not the bullet's. You're right - the bullet should be dumb. Its single responsibility is to call takeDamage (or perhaps a better named onHit or something) with whatever entity it hits. That's it. It's up to the entity to decide how to deal with that. Then your player's takeDamage function should be something along the lines of your final function, only with respect to its internal state:

takeDamage: function(damage) {
    this.playHitSound();
    if(!this.canTakeDamage()) {
        this.applyDamage(damage);
        this.lastHitTimer.reset();  // although this should be part of applyDamage, perhaps
        this.checkIfDead();
    }
}

A good way to think of this is to consider how you might want to extend your entity into a subclass in the future, and break things down such that you can only change one part. For example: suppose I wanted to make an entity that had a third type of shield, how would I do that? Or, I want to make an entity that takes a maximum of 2 damage from any attack. Or I wanted the entity to not take damage if they were invincible, but still get knocked back. So think of some use cases, and once you have an API that makes those easy to change, chances are it's pretty solid.

It's really great that you are thinking about these things.

Great response!
Thank you mdkess.
Will likely take that approach then, and make sure that the bullet stays dumb.