Is this bad code?

Question

I've been using this code style for a while now, I normally make a class that has "ID" in front of it, place it in a header, then make a .cpp file and put a class called "IDClassnameLocal". I make pure virtual functions in the abstract header class, then make normal virtual functions in the .cpp class and have it inherit the abstract header class.

Is this bad code design and am I coding efficiently?

I put ID(identification) in front of the class name to prevent redefinition and for cleanness.

Example:

// Player.h // ////////////////////////////////////////////////////////////////
class IDPlayer {
public:
    virtual ~IDPlayer(void) {} // Destructor

    virtual void PlayerData(void) = 0;
    virtual void Controls(void) = 0;
};
extern IDPlayer* idplayer;

// Player.cpp // //////////////////////////////////////////////////////////////
class IDPlayerLocal : public IDPlayer {
public:
    IDPlayer(void) {} // Constructor

    virtual void PlayerData(void);
    virtual void Controls(void);
};
IDPlayerLocal idplayerLocal;
IDPlayer* idplayer = &idplayerLocal;
// Class Function Definitions
void IDPlayerLocal::PlayerData(void) {
    Player.X = 400;
    Player.Y = 500;

    Player.W = 20;
    Player.H = 20;

    Player.VelocityX = (float)0.31;
    Player.VelocityY = (float)0.31;
}

void IDPlayerLocal::Controls(void) {
    if(::MainGame) {
        if(KEY_DOWN(0x41)) { // A
            Player.X = Player.X - Player.VelocityX;
            if(Player.X <= 0)
                Player.X = 0;
        }
        if(KEY_DOWN(0x44)) { // D
            Player.X = Player.X + Player.VelocityX;
            if(Player.X+Player.W >= 650)
                Player.X = 650 - Player.W;
        }
        if(KEY_DOWN(0x57)) { // W
            Player.Y = Player.Y - Player.VelocityY;
            if(Player.Y <= 0)
                Player.Y = 0;
        }
        if(KEY_DOWN(0x53)) { // S
            Player.Y = Player.Y + Player.VelocityY;
            if(Player.Y+Player.H >= 570)
                Player.Y = 570 - Player.H;
        }
        if(KEY_DOWN(VK_SPACE)) {

        }
    }
}


// Core.cpp // ////////////////////////////////////////////////////////////////

// ...
// Intialized Data //
idplayer->PlayerData();

while(TRUE) {
    // ...
    // Loop Data //
    idplayer->Controls();
    // ...
}
// ...

@DavidSchwartz It means "identification". Just a typical abbreviation and keeps things organized. I added a note in the question explaining why. — Tristan Gibson, Apr 06 '13 at 01:38
@TristanGibson use namespaces. This is C++, not Objective-C. — , Apr 06 '13 at 01:42
@Zoidberg I'll keep that in mind, thanks. Also, I've never used Objective-C. — Tristan Gibson, Apr 06 '13 at 01:44
@TristanGibson The only "real" answer to that question is [get a book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list). — Etienne de Martel, Apr 06 '13 at 01:49
@EtiennedeMartel Wouldn't that apply to almost every question? — Tristan Gibson, Apr 06 '13 at 01:52
I'm just confused as to why you would do all this? Don't misunderstand - sometimes this sort of thing makes sense, but it doesn't make sense *all the time*. — Nik Bougalis, Apr 06 '13 at 01:57
@TristanGibson It sounds every class you create is designed as an interface. Why? Sometimes that makes sense, but not all the time. If this your "standard" design something is, almost certainly, wrong. — Nik Bougalis, Apr 06 '13 at 02:24
@NikBougalis I'm not sure when and when not to make a class an interface. Maybe when I use the abstract class for multiple classes to derive? — Tristan Gibson, Apr 06 '13 at 02:29

score 5 · Accepted Answer · answered Apr 06 '13 at 01:35

5

As long as you are using the same style throughout your code no style is really bad unless unreadable. I was able to go through your code without so much trouble so I would say it is a good style. Just be consistent throughout your project.

answered Apr 06 '13 at 01:35

iordanis

1,284
2
15
28

Do you know if it's efficient? – Tristan Gibson Apr 06 '13 at 01:42
@TristanGibson Define "efficient". – Etienne de Martel Apr 06 '13 at 01:48
3

@TristanGibson : Making everything virtual for no apparent reason is certainly _not_ efficient. – ildjarn Apr 06 '13 at 01:50
@ildjarn Well, they are virtual because the .cpp class is deriving them. – Tristan Gibson Apr 06 '13 at 01:56
@TristanGibson Something tells me you aren't very clear on C++ and don't quite understand the point of class derivation or virtual functions. You can have derived classes without needing virtual functions. – Nik Bougalis Apr 06 '13 at 01:59
1

@TristanGibson : And why is there a derived class to begin with? Why not just a single class, declared in a header, and defined either in the same header or in some .cpp? – ildjarn Apr 06 '13 at 02:01
@Danny And I would disagree - being consistent is a good thing no doubt, but you can be consistently bad which... well, just isn't good. – Nik Bougalis Apr 06 '13 at 02:03
@ildjarn I don't know when I should use derived classes and I also don't really understand why I put virtual functions in them. I just read that was what I was supposed to do. – Tristan Gibson Apr 06 '13 at 02:10
1

@TristanGibson : You read that about C++, or some other language? For non-COM-oriented _C++_ this is a rare approach, unlike, say, Java. :-] – ildjarn Apr 06 '13 at 03:19

score 1 · Answer 2 · answered Apr 06 '13 at 01:47

1

What you are doing may be useful in certain cases, but certainly not for all classes. You need to consciously decide whether this is appropriate for your class. So adopting this approach as a 'code style' would be a bad thing.

answered Apr 06 '13 at 01:47

StackedCrooked

34,653
44
154
278

score 0 · Answer 3 · answered Apr 06 '13 at 01:53

You don't need to comment constructors or destructors. Nor the name of the file but here it is ok we can't see what blocks goes to what file without them
extern IDPlayer* idplayer is a global. It smells bad but might be ok
IDPlayer* idplayer = &idplayerLocal Is there a reason you don't use a reference (IDPlayer& idplayer = idplayerLocal)
X, Y, W, H seems weird to be in caps. Usually caps have special meanings. I don't think you should do that
(float)0.31 is weird. Either plainly do 0.31 or use a suffix like f (float) or d (double) (I think I can't remember my suffix).
::MainGame another global but in games i do have a global so its fine. But do you really need to use ::? Its not bad but seems funny i wouldnt think to search if(::
Not a problem but are you sure you want W and S, A and D to cancel eachother out?
Is KEY_DOWN a macro? why isnt it a function? Also instead of hex code of the ascii letter just use KEY_DOWN('A')
Usually I have while(gameRunning) and not true. Unless you have complicated ifs to break out of the loop then its good to use while(true) or while(1)

It's using DirectX and Win32 API. I normally use :: global scoping because it's easier for me to recognize globals. KEY_DOWN is a constant: #define KEY_DOWN(vk_code) ((GetAsyncKeyState(vk_code) & 0x8000) ? 1 : 0) #define KEY_UP(vk_code) ((GetAsyncKeyState(vk_code) & 0x8000) ? 0 : 1) — Tristan Gibson, Apr 06 '13 at 02:01
oh right i forgot my windoze api. It was actually not that difficult much of the time. — , Apr 06 '13 at 02:04
"Is there a reason you don't use a reference" Should I use a reference instead? — Tristan Gibson, Apr 06 '13 at 02:15
@TristanGibson: Yes, if it never will be null, should always exist and won't change use a reference. The only time I don't use a reference is when a 3rd party library force me not to. — , May 03 '13 at 03:34

Is this bad code?

3 Answers3