hi
it looks nice game. i like it.
i have looked your code. i think there are many things you may need to do. i recommend you to try this site
codereview.stackexchange.com to give you better code review.
however. i will briefly review your code, btw, i'm still beginner i just learnt c++ last years.
. for comprehensive code review check the site above.
in your Enemy class- it is better to put a class declaration and definition in two separate files like .cpp and .h or .hpp.
- it's preferable to use float for position and time calculation rather than int.
- use const for member data that won't change throughout game like speedPerSecond, maximumDistance
- no need for "this->" pointer, you may use Constructor initialization lists for members data
here Enemy class after modification:
Enemy.h#include <SFML/Graphics.hpp>
class Enemy final : public sf::Drawable
{
public:
Enemy(const sf::Texture& texture, float posx, float posy);
// usually the member function start with small letter in c++
// also, it uses in SFML
void update(float speedX, float timeDelta);
sf::FloatRect getGlobalBounds() const;
bool isAlive() const;
private:
// draw can be declare as private
void draw(sf::RenderTarget &target, sf::RenderStates states) const override;
private:
// there are many style guides for c++ most known and widely uses is camelCase
// for member data. personally i like camelCase over snake_case. in Both of them thay
// used 'm' prefix to indecate the data is member data like below
sf::Sprite mSprite;
const float mSpeedPerSecond;
float mDistanceTravelled;
const float mMaximumDistance;
};
Enemy.cpp#include "Enemy.h"
Enemy::Enemy(const sf::Texture& texture, float posx, float posy)
: mSprite(texture)
, mSpeedPerSecond(20.f)
, mDistanceTravelled() // by default is zero
, mMaximumDistance(472.f)
{
mSprite.setPosition(posx, posy);
}
void Enemy::update(float speedX, float timeDelta)
{
auto distanceTravlledThisUpdate = mSpeedPerSecond * timeDelta;
mDistanceTravelled += distanceTravlledThisUpdate;
mSprite.move(speedX*timeDelta, distanceTravlledThisUpdate);
if (mSprite.getPosition().x > 1600)
{
mSprite.setPosition(-800, mSprite.getPosition().y);
}
if (mSprite.getPosition().x < -800)
{
mSprite.setPosition(1600, mSprite.getPosition().y);
}
}
sf::FloatRect Enemy::getGlobalBounds() const
{
return mSprite.getGlobalBounds();
}
bool Enemy::isAlive() const
{
return mDistanceTravelled < mMaximumDistance;
}
void Enemy::draw(sf::RenderTarget &target, sf::RenderStates states) const
{
target.draw(mSprite, states);
}
in Game classit should be separated in .cpp and .h files
you may use std::unique_ptr instead of std::share_ptr.
std::unique_ptr<Enemy> eEnemy(new Enemy(enemy, posx, posy));
enemies.push_back(std::move(eEnemy));
i don't really like loop inside loop for the vector of enemies in this snippet
for (auto i = enemies.begin(); i != enemies.end(); i++)
{
if (!(*i)->IsAlive())
{
// no need for loop whole enemies vector in here
enemies.erase(std::remove_if(enemies.begin(), enemies.end(), [](const std::shared_ptr<Enemy>& o) { return !o->IsAlive(); }), enemies.end());
hitTheGround++;
enemiesLeft--;
break;// <-- maight be a bug
}
else
{
(*i)->Update(speedX, timeSinceStart.asSeconds());
}
}
it would be much easier if you just make it like this:
for (const auto& enemy : enemies)
enemy->update(speedX, timeSinceStart.asSeconds());
enemies.erase(std::remove_if(enemies.begin(), enemies.end(),
[this](const std::unique_ptr<Enemy>& o)
{
if (!o->isAlive())
{
hitTheGround++;
enemiesLeft--;
return true;
}
else
return false;
}), enemies.end());
why you call collision() multiple times in game-loop, only one call is enough.
backgroundLoop();
collision();
render();
createBullet();
collision();
createEnemies();
collision();
processEvent();
also, no need for "enemiesLeft" you can check the enemies left by vector size. i don't know if your compiler support std::to_string(), if so, you can use it like this:
showEnemiesLeft.setString("Enemies left: " + std::to_string(enemies.size()));
and there are more but for time being it is better to concentrate on code styling and files organizing
