A few common code smells to be aware of and how to fix them

A code smell is something in your codebase that may be indicative of a larger problem in the system. I use the word “may” here because sometimes there is a legitimate reason for code like this to exist, but just like if you smell smoke you should probably make sure the house isn’t on fire, if you come across a code smell you should take a moment to evaluate it and see if your code needs a bit of care and attention. So here’s a few common code smells that you may find in your games, why they can be concerning, and how to fix them.

Long functions

Long functions are probably one of the most common code smells you’ll come across. We all write them from time to time because when you’re in the zone, you’re just focused on getting the code out and don’t want to break that flow by focusing on something else, like organization. That’s alright, but you should come back to it later and make sure the function is an appropriate length. I don’t like to offer specific length recommendations, as that just results in an arbitrary cutoff point, but if you’ve got more than ten or twenty lines of code in a function, ask yourself if all of the function’s code is related to doing a single thing or if if things could be broken out into smaller functions.

We want to do this so our code is easy to read (long blocks of code are more difficult to read), easy to maintain (fewer side effects in a smaller function), and isn’t duplicated (if function A does three things and I only need one of them in function B, I now have to duplicate just the code that I need in function B, which is a bad thing). The last point about code duplication is, in my opinion, one of the largest reasons to pay attention to this code smell as it also implies that code reusability is poor.

For instance, a function to calculate how much damage to take shouldn’t also take care of everything else related to taking damage, such as modifying the unit’s health, updating the UI, playing a sound effect, and emitting some particles, but it CAN be incorporated into another, general purpose, take_damage() function that coordinates these tasks.

Bad - This function is doing many unrelated things in a way that’s not resusable

function calculate_damage(attack) {
    // Calculate damage
    var total_defense = stats.base_defense + stats.modifiers.defense
    var damage = attack.damage * total_defense
    if (stats.resistances.has(attack.type)) {
        damage *= stats.resistances[attack.type].modifier
    }
    else if (stats.weaknesses.has(attack.type)) {
        damage *= stats.weaknesses[attack.type].modifier
    }

    // Update health, check for death, and update UI
    var old_health = health
    health = clamp(health - damage, 0, max_health)
    health_bar.interpolate(old_health, health)
    if (health <= 0) {
        is_alive = false
        play_animation('death')
    }

    // Apply effects
    damage_sfx.stop()
    damage_sfx.position = 0
    damage_sfx.play()

    damage_particles.emit()
}

Good - Each task involved in taking damage is organized into its own reusable function

function take_damage(amount) {
    var final_damage = calculate_damage_dealt(amount)
    update_health(final_damage)
    update_health_bar()
    play_sfx('damage')
    emit_particles('damage')
}

Long parameter lists

Another common issue is having long parameter lists for functions, and there’s really two main issues with this. The first is that it makes your code less readable, as it probably doesn’t all fit into a single line on your monitor, and the other is that it implies this function is too complex in one way or another. Maybe the function is trying to be too generic, branching internally based on what parameters it receives, and therefore requires a lot parameters that it doesn’t need in every invocation, or maybe the data being passed in really is this complicated, but should be contained in a data object of some sort to make it easier to pass around, as gathering ten separate parameters every time you need to call a function is going to add a lot of bulk to your code and open up the potential for data to become out of sync in some way. For instance, this data object technique is commonly used in a variety of APIs and SDKs to house optional parameters on functions that really do need to be fairly generic.

Bad - Too many parameters are required that may not always be used

function get_nearest_object(search_radius, search_method, include_enemies, include_players, include_static_objects, consider_disabled_objects) {
    if (include_enemies) {
        ...
    }

    if (include_players) {
        ...
    }

    if (include_static_objects) {
        ...
        if (consider_disabled_objects){
            ...
        }
    }

    ...
}

Good - Restructuring our code to be more specific results in cleaner function calls

Note that in this example you may still have a shared core function that handles the search calculations, but assuming there IS a difference in pre and post processing, that’s where the benefit of breaking things into separate functions comes into play.

function get_closest_enemy(search_radius, search_method) {
    //...
}

function get_closest_player(search_radius, search_method) {
    //...
}

function get_closest_static_object(search_radius, search_method) {
    //...
}

Good - Using an object to represent search options for a generic function

function get_nearest_object(search_radius, search_options) {
    //...
}

Duplicate code

In case it’s not apparent, duplicating code is something to be avoided, and can be an easy to catch smell that implies the codebase may have been a bit hastily put together.

The first time you copy a few lines and then later have to make a change to it wherever it occurs is enough to make you recognize the importance of keeping code unique. Consider the following code snippet:

function take_melee_damage(amount) {
    var damage = amount * defense_modifier
    ...
}

function take_magic_damage() {
    var damage = amount * defense_modifier
    ...
}

function take_environmental_damage() {
    var damage = amount * defense_modifier
    ...
}

If we want to add an additional damage modifier (for elemental resistance, for instance), we’ll have to adjust our damage calculation code in all three functions. This is time consuming to fix, and if the duplicated code isn’t side by side in our application, we’re likely to miss a change to it in the future, resulting in weird bugs where, for instance, damage is calculated incorrectly only when the player is standing in a fire started by an enemy.

If the same code is needed throughout a single script, turn it into a function. If a lot of different objects need the same functionality, ask yourself if they’re similar enough to inherent from a shared class. If not, another option is to create a set of utility functions that house commonly needed code, such as formatting currency or interpolating between two values. And if none of those options feel suitable, perhaps a reusable component that can be attached to a wide variety of objects is the right call.

God objects

So you’ve got your player managing their health internally when they heal or take damage, and now you need to update the UI. You skipped my post on the observer pattern, so instead you just create a global object and use it to hold a reference to the player so the UI can ping for health changes every frame. Not exactly elegant, but it works. Then you need you decide you want to show a score based on how many enemies have been killed, so you put a few more references into this global object. Then you need a few helper functions to manage things and pass data around so you add that, and you might as well make it so you can play a few sound effects from within this object since you’re already passing health and enemy data around and it’s easy to catch when something is happening. Plus, you haven’t gotten around to really figuring out how to handle audio yet so where else would it go?

Next thing you know, you’ve got a god object on your hands: an object with no singular purpose that essentially acts as a dumping ground for data and functions you couldn’t decide where to otherwise place. This is problematic, as global data and objects are the singular strands that pile up into spaghetti code, making it difficult to debug and reason about your game’s state since significant changes to the data and function calls can occur from anywhere in the app. Plus, once you’ve gone down this path, it can make it hard to get out of it. Refactoring global data to non-global data generally isn’t going to be a trivial task, and the constant temptation of making just one more addition to the existing dumpster fire is going to be hard to resist.

So ask questions, study a few design and coding patterns, and don’t dump everything into a singular global object.

Overengineered solutions

For how much I talk about good coding form and the importance of design patterns, it feels important to end with a brief bit about the other side of the spectrum: Don’t overengineer things. Building a solution that’s much more complex and difficult to maintain than what’s needed is just as bad, and sometimes worse, than having an underengineered solution. Sure, that function is a bit long, but that doesn’t mean you need to make an entire class with multiple layers of inheritance just to get the length down. Deep dependencies like that are difficult to maintain, difficult to debug and reason about, and can often add dead code that’s not actually needed.

The goal is to make your code flexible enough that you can add to it or refactor as needed later on, but not to take every feature as an exercise in designing an overly abstract, convoluted architecture that may never be required. I will probably discuss this point more in depth in a later post, but here’s a few questions I like to ask myself when figuring out what I need:

• What features are required? These need to be prioritized in the design.
• What features are reasonably likely to be needed? This could be something like an optional game mode I want to add if time allows, a new feature that’s still in the prototyping stage, etc. I’ll keep these features in mind when coding, but their inclusion can’t override what I know I need and I won’t take much time or effort to support them until I know I need them. This is the place to pick the low hanging fruit: parameterize a function, create an enum to represent different configurations, etc.
• What’s the scope and impact of the code? High impact code, such as the structure of a level, should get more attention than code that is low in both scope and impact, such as the callback for a specific button on one menu that will never reused.

Remember, simplicity is often the best choice.