• Advertisement
Sign in to follow this  

How can this code be refactored to be more proper OOP?

This topic is 976 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

How can the code below be refactored to remove the set and get methods? And more importantly should they be and why?

// Code taken from somewhere
 
 
#include "app.h"
#include "enemy.h"
 
Enemy::Enemy(App* app, const char* file,int screenWidth, int screenHeight, SDL_Renderer* renderer) {
        x = 0;
        y = 0;
        w = 0;
        h = 0;
        fireTicks = 0;
        fireRate = 400;
        velocity = 250;
        alive = false;
        enemyBulletTex = NULL;
        this->app = app;
 
        if((enemyTex = gfxManager::loadTexture(renderer,file)) == 0) {
                throw std::invalid_argument("err, could not load the enemy texture!");
        }
 
    if((enemyBulletTex = gfxManager::loadTexture(renderer, "../gfx/enemyBullet_1.png")) == 0) {
        throw std::invalid_argument("err, could not load the enemyBullet texture!");
    }
 
    SDL_QueryTexture(enemyTex, NULL, NULL, &w, &h);
 
    x = (screenWidth - w) /2;
    y = 150;
}
 
Enemy::~Enemy() {
}
 
void Enemy::think(float elapsedSeconds) {
    if(isAlive()) {
        shoot();
 
        // boundaries detection
        if(x < 0) {
            x -= x;
        }
 
        if(x + w > app->getWidth()) {
            x -= (x + w - app->getWidth());
        }
    }
}
 
void Enemy::render(SDL_Renderer *renderer) {
    if(isAlive())
        gfxManager::drawTexture(renderer,enemyTex,x,y);
}
 
void Enemy::shoot() {
    int currentTime = SDL_GetTicks();
    if(fireTicks + fireRate < currentTime) {
        for(int i = 0; i < app->getBulletPoolSize(); i++) {
            if(app->getBulletPool()[i]->getActive() == false) {
                app->getBulletPool()[i]->setActive(true);
                app->getBulletPool()[i]->setTexture(enemyBulletTex);
                app->getBulletPool()[i]->setX((x + w / 2)-app->getBulletPool()[i]->getW()/2);
                app->getBulletPool()[i]->setY(y + h / 2);
                app->getBulletPool()[i]->setOwner(-1);
                break;
            }
        }
        fireTicks = currentTime;
    }
}
 
void Enemy::setAlive(bool status) {
    alive = status;
}
 
bool Enemy::isAlive() {
    return alive;
}
 
int Enemy::getX() {
    return x;
}
 
int Enemy::getY() {
    return y;
}
 
int Enemy::getW() {
    return w;
}
 
int Enemy::getH() {
    return h;
}
Edited by Alpha_ProgDes

Share this post


Link to post
Share on other sites
Advertisement

I would remove them and possibly add methods to obtain a position and a boundingbox (or a more generic collider) instead, setters shouldn't be needed imo, if you want a public method for moving the enemy it should probably validate the new position aswell.

 

I would also remove the gfx loading and rendering code from the enemy and give it a sprite id instead.

 

The shooting code could also be refactored so that you ask the bullet pool for an unused bullet rather than having your enemy be responsible for looping over it.

 

i.e, bullet = app->bulletPool->getOrCreateBullet(); do stuff with bullet

Edited by SimonForsman

Share this post


Link to post
Share on other sites

Its probably a good idea to have a way to get/set the enemy position. Sometimes its good to have flexibility even if you dont need it at the moment, especially when it comes to gameplay.

(edit: though, consider whether the position and such should be modified inside a method of enemy, or externally, something like movement logic is probably better to be encapsulated inside enemy)

 

Some comments:

 

-You currently duplicate lots of code because the x and y dimensions are treated separately, each being its own variable.

I like to use a vector class so I need just one variable that contains both the x and y dimension - you often want to do vector math and pass vectors, not operate on individual axis values, so they almost always belong together as a logical unit. This could be as simple as struct Vector2i {int x; int y;}; or something more advanced (I use a template so it works for any number of dimensions and primitive type). The benefit here is you need just one set/get pair for position instead of two (same for passing as function parameter).

 

-Mark the methods that dont modify member variables as const: "int Enemy::getH() const {return h;}" (const correctness). Makes it easier to reason about the code when theres less methods with side effects, and otherwise your Enemy class wont be very useful if you make a const instance/reference of it.

 

-In the shoot method, you might want to move the logic for initializing a new bullet into "app" since it seems like something that will be used by any bullet shooting entity (as a method of app).

Edited by Waterlimon

Share this post


Link to post
Share on other sites

Okay, so to pour some clean wine... That code Alpha provided here is mine.. for my hopefully upcoming game Neon Flyers. Well we were discussing about Getters and Setters, and  I showed my code to Alpha and in this way he "overrunned" me and posted the code here.. That is okay but.. Id like to understand why in these modern ways of C++. Making getters and setters is not such a good idea for private members.. Well I understand that pivate member with public get/set methods is like having it public. But I always was taught my online tutorials etc.. that members in class should always be private or protected, if they are inherited.

So my question is how to work with it.. and how to avoid messing with get/set methods in such quantitive way as I have above.

 

As I see all your answers here it might be good if you had access to whole project but... project is in rewriting process, old repo is down, new has been made..  But true is that there already might be some misstakes done - considering to your asnwers here. https://bitbucket.org/Tetriarch/neon-flyers/src/9d05dd46d60c1f0a0ed32ed2aaf8997cf5ecbc40/neon-flyers/?at=master
I see the fact that I have done wrong decision, messed responsibilites of enemy class etc.. 

 

Well to be honest with such answer, which I dont claim is bad, because it ain't I have a bit problem to deal in matter of imagination how such code should look.. So I am asking whether there can somebody at least in some diagram or pseudo code show me how could I achieve that? So the project would be more object oriented?? 

Thanks a lot for help, Tetriarch.

Edited by Tetriarch

Share this post


Link to post
Share on other sites


Well I understand that pivate member with public get/set methods is like having it public

 

The way I see it, getters/setters are different because they allow you to change the underlying data type, or easily add some logic to happen when the position is set or get. Eg maybe you need to update the position of a physics collider when the entity position is changed (lets say it uses floats so you still need a separate position in entity)

Or maybe you need to add some sanity checks or logging or whatever.

Without having to change any of the code that uses your class, that is.

Share this post


Link to post
Share on other sites

For this code, isn't there a better way to represent or refactor it out of the code completely?

void Enemy::setAlive(bool status) {
alive = status;}
bool Enemy::isAlive() {return alive;}
int Enemy::getX() {return x;}
int Enemy::getY() {return y;}
int Enemy::getW() {return w;}
int Enemy::getH() {return h;}

Share this post


Link to post
Share on other sites
Depends on the details.

The reason that usually works so well in C++ is that these non-virtual functions can be inlined if you implement them correctly by placing them in the header and feeding good compiler options. Thanks to the magic of optimization the data can be read directly with a single cpu instruction instead of actually calling a function, passing parameters and return values, or doing other work.

It works less well in situations where the compiler cannot do that. If they are virtual it still requires a virtual dispatch as well as a function call. If they are placed in the implementation file rather than the header the tools usually won't optimize the call away requiring a full function call.

All the work of passing parameters and return values, function call setup/teardown, and potentially virtual dispatch can turn a direct load -- potentially a fraction of a nanosecond -- into an full call -- potentially several hundred nanoseconds. Multiply all those nanoseconds by tens of thousand or hundreds of thousands of calls and very quickly you're talking about a big chunk of processor time.


But that optimization of inline accessors and mutators isn't the big thing here. Usually 'object oriented' means thinking about systems and how objects move through them. Rendering objects move through a rendering system. Game objects move through a game simulator system. Networking objects move through a serialization, data transfer, and deserialization system.

Thinking in terms of systems, the textures tend to fit within a graphics system, positional information tends to fit within a spatial and physics system, AI tends to fit within the simulator system. Often it can make sense to have a game object link together multiple systems through composition. The object has a reference to its spatial representation, it has a reference to its graphics representation, it has a reference to whatever else it needs.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement