Space Shooter: Multiple weapons approach

Question

I'm building a small space shooter and want my player to have multiple weapons available. I'm using an array and a vector for the weapons (both initialized when the player is initialized). Both the weapon array and vector are members of the player.

    CWeapon     m_aWeapons[3];  // Array for unlockable weapons
    vector<CWeapon> m_vWeapons; // Vector for unlocked weapons

The weapons that can be unlocked are initialized in an array at the beginning.

// Set unlockable weapons
m_aWeapons[0].Init("LaserOrange.bmp", 0.2f, 400.0f, 20.0f);
m_aWeapons[1].Init("LaserBlue.bmp", 0.2f, 600.0f, 20.0f);
m_aWeapons[2].Init("LaserRed.bmp", 0.2f, 800.0f, 20.0f);

I save the weapons the player has available at current in the vector.

After each level the next weapon is unlocked, if possible, by pushing the next element of the array into the vector with the public method CPlayer::UnlockWeapon ().

// Unlock
//
// Task: Unlock next weapon
//
bool CPlayer::UnlockWeapon()
{
// Unlocked new weapon?
bool bUnlocked = false;

// Temp for size of weapon vector and array
int VectorSize = static_cast<int> (m_vWeapons.size ());
int ArraySize = sizeof(m_aWeapons) / sizeof(CWeapon);

// Not unlocked every weapon?
if (VectorSize < ArraySize)
{
    // Yes, then unlock next
    m_vWeapons.push_back(m_aWeapons[VectorSize]);

    // Unlocked new weapon
    bUnlocked = true;
}

return (bUnlocked);

} // UnlockWeapon

I save the index of the currently selected weapon in a variable and when the player shoots, I create a shot and initialize it with the speed and shotsprite of the vector element at the index of the currently selected weapon variable.

// ProcessShooting
//
// Aufgabe: Shoot weapon
//
void CPlayer::ProcessShooting ()
{
// Hasn't shot until now
m_bHasShot = false;

// Pressed "Space" and can shoot?
if (g_pFramework->KeyDown (SDL_SCANCODE_SPACE) && m_bShotLock == false)
{
    // Has shot
    m_bHasShot = true;

    // Create and initialize new shot
    CShot Shot;

    // Temp for ease
    CWeapon Temp = m_vWeapons.at(m_CurrentWeapon);

    Shot.Init(Temp.GetSprite(), m_fXPos, m_fYPos, Temp.GetSpeed ());

    // Insert shot into shotlist
    m_ShotList.push_back (Shot);

    // Shoot again, when space released
    m_bShotLock = true;

}

// Released space, so allow shooting
if (g_pFramework->KeyDown (SDL_SCANCODE_SPACE) == false)
    m_bShotLock = false;

} // ProcessShooting

Is that a strange approach or can I stick with it?

Answer 1

It looks strange to me that you're copying the CWeapon objects everywhere: they look very much like the typical Type Object pattern, where an instance is immutable and represents an in-game type of weapon. In fact, I suggest renaming CWeapon to WeaponType and making it non-copiable. Then you pass around pointers to weapon types to use them, no copies needed.

You also apparently don't use constructors, which would be a good habit to get into: Init functions are neither needed nor practical in C++.

Refactoring your snippets, with some more inline tweaks, leads to:

Player.h

class Player {

    // ...

    // std::array rather than C-style array
    // Also consider std::vector, even if the size doesn't change,
    // for you then don't need to update it when adding weapons.
    // Also made the member static (no need to duplicate it in various players)
    // and const (no apparent need to tweak weapon types on-the-fly).
    static std::array<WeaponType, 3> const unlockableWeapons;

    // Clear naming makes comments redundant
    std::vector<WeaponType const*> m_unlockedWeapons;

};

Player.cpp

std::array<WeaponType, 3> const Player::unlockableWeapons{{
    {"LaserOrange.bmp", 0.2f, 400.0f, 20.0f},
    {"LaserBlue.bmp",   0.2f, 600.0f, 20.0f},
    {"LaserRed.bmp",    0.2f, 800.0f, 20.0f}
}};

bool Player::unlockWeapon() {

    auto const alreadyUnlocked = m_unlockedWeapons.size();

    // Simplify your control flow by returning early
    if(alreadyUnlocked == unlockableWeapons.size())
        // Everything is already unlocked
        return false;

    // Unlock the next weapon
    m_unlockedWeapons.push_back(&unlockableWeapons[alreadyUnlocked]);
    return true;
}

void Player::processShooting() {

    m_hasShot = false;

    if(!g_framework->keyDown(SDL_SCANCODE_SPACE)) {
        m_shotLock = false;
        return;
    }

    auto *const currentWeapon = m_unlockedWeapons[m_selectedWeapon];

    // Using emplace_back to construct the shot directly into the vector
    m_firedShots.emplace_back(
        currentWeapon.getSprite(),
        m_xPos, m_yPos,
        currentWeapon.getSpeed()
    );

    m_shotLock = true;
}

I've also tweaked the naming convention, which looked like it came from MFC. Usual (in my experience -- moderately broad but not a hard truth) C++ naming is:

• PascalCase with no prefix for classes;
• camelCase for functions;
• camelCase with prefix for non-public member variables. m_ and _ are two popular prefixes.
• No systems hungarian notation: C++ has types, no need to repeat them in the variable's name.

Yes, the standard library is a notable exception, which uses snake_case everywhere.