Is something like this ok?

Started by
12 comments, last by _Sigma 18 years, 8 months ago
Ever since I learnt about classes, I've loved using them. But in my game, I think I may have gone a little overboard. Is this ok?

DDback->Blt(&Player.Animation_Walk_Up.Sequence[Player.currentFrame].dest_rect,Player.Animation_Walk_Up.Sequence[Player.currentFrame].DDTile, &Player.Animation_Walk_Up.Sequence[Player.currentFrame].src_rect,DDBLT_WAIT | DDBLT_KEYSRC,NULL);

OR

 Player.Animation_Walk_Right.Sequence[Player.currentFrame].dest_rect.left = Player.x;
The DDback->Blt() is DirectX, so it doesn't count. But, the rest is mine. Is that way too many objects? Cheers
Advertisement
Whether or not it is too many objects depends on the details of the project.

I think it would be better to write a setting method, with a descriptive name, in the interface of the class to encapsulate all the indirection to other member classes.
The problem is that it is a nightmare to read. And there doesn't appear to be any encapsulation of data at all, which would get a lot of people riled up.

It would probably be more generally acceptable to say something like...

Animation *anim = player->getAnimation[ANIM_WALK_RIGHT];Sequence *seq = anim->getSequence( player->getCurrentFrame() );seq->setDestRec( /* whatever */ );


Of course, I'm just making type names up and stuff, but this sort of thing is much more readable in my opinion.
Quote:Original post by _Sigma
But in my game, I think I may have gone a little overboard. Is this ok?


It's rarely ok to go overboard with anything. [wink]
But you're the one who have to deal with your code. So you'll have to decide if it's ok.
Looks messy to me though.

But yeah, what happened to encapsulation?
..encapsulation?? Anyone care to explain?
Greatly simplified, encapsulation here means that you should hide implementation of specific code (this case: rendering of a player sprite) from the end-user. This helps to concentrate on the task you actually want to do; Just render the sprite, nothing else.

Here's how I do it in my sprite code:

CGameObject Player; // Create our sprite
Player.MoveRel(-1, 0); // Move 1.0 to left in relative coordinates
Player.SetAnimation(ANIM_WALK_LEFT);
Player.Render(); // ... And render him

You see how data is being hidden from the coder? Talk about cool huh :)
"Encapsulation" means that access to an objects members is restricted to only those accesses that you deem appropriate.

For example... Let's change one of the lines that you used...
Player.Animation_Walk_Right.Sequence[Player.currentFrame].dest_rect.left = -2849309485.2945;

Is that what you want? Probably not... if you only allow access to that variable through an accessor method you can validate the values, etc.

sequence->getDestRec()->setLeft(-2849309485.2945);

Now, the setLeft() method can do some validation on the value.

Generally speaking, if someone (a potential employer, perhaps) looks at your code and sees you directly accessing any class members like you were doing, they will think less of you as a programmer.

Like someone else said, though, if this is just for you and you don't care what anyone else thinks then it doesn't really matter.

It's a good idea though to have getter/setter methods for all of your data, and if I were you I would try to get in the habit.
ALright, I think I follow. So something like this then?
Sprite Player...Sprite::Render(Int Direction){  case(Direction)  {      case Right:                 DDback->Blt(&Player.Animation_Walk_Up.Sequence[Player.currentFrame].dest_rect,Player.Animation_Walk_Up.Sequence[Player.currentFrame].DDTile, &Player.Animation_Walk_Up.Sequence[Player.currentFrame].src_rect,DDBLT_WAIT | DDBLT_KEYSRC,NULL);break;[other cases here]}}...Player.Render(RIGHT);

So something like that then eh?

PS> How do you do the thing again?
Also, how hidden should "bad" code like that be? :P
There's no need for all that repetition (copy'n'pasting). At least do something like this:
const Sequence &seq = Player.Animation_Walk_Up.Sequence[Player.currentFrame];DDback->Blt(&seq.dest_rect, seq.DDTile, &seq.src_rect, DDBLT_WAIT | DDBLT_KEYSRC, NULL);
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

This topic is closed to new replies.

Advertisement