• Advertisement
Sign in to follow this  

Is something like this ok?

This topic is 4585 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

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

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites
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 :)

Share this post


Link to post
Share on other sites
"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.

Share this post


Link to post
Share on other sites
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 [code=cpp] thing again?

Share this post


Link to post
Share on other sites
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);

Share this post


Link to post
Share on other sites
Quote:
Original post by _Sigma
Also, how hidden should "bad" code like that be? :P
Quite the opposite.
Do not hide bad code. Preferably do not write bad code. But it you ever do then make it plainly obvious with plenty of comments about how bad it is, and a big loud flashing sign that says FIXME![grin]

Share this post


Link to post
Share on other sites
sorry, by bad, i meant, ugly, stuff like what I had :P

I now have all that stuff within a single function Sprite::Render(); I just call it from my main game loop. If I add what was suggested by iMalc (and others :P) i should be ok, and not considered a nub if I realease the code? :P I'm taking compsci next year(1st year uni) , so i want to learn good habits now.

Share this post


Link to post
Share on other sites
Moving bad code around isn't about hiding it, it's about fixing it. The other piece of the puzzle is to identify the redundant bits and extract common functionality so that you don't have to write it multiple times. Moving things around helps to find redundancy (by putting redundant bits next to each other) and also to extract the functionality (by making it easier to decide where the common implementation logically goes).

In your case, the proposed Sprite::Render would not work with a "Sprite Player", but instead directly with the "this" object (and you only have to write "this-> in cases where there may be ambiguity). With a bit of rearrangement of whitespace, we get something like:


enum {UP, RIGHT, DOWN, LEFT} Direction;

void Sprite::Render(Direction d) {
switch (d) {
case UP:
// "Animation_Walk_Up" and "currentFrame" are implicitly the members of
// the Sprite upon which this is called.
DDback->Blt(&Animation_Walk_Up.Sequence[currentFrame].dest_rect,
Animation_Walk_Up.Sequence[currentFrame].DDTile,
&Animation_Walk_Up.Sequence[currentFrame].src_rect,
DDBLT_WAIT | DDBLT_KEYSRC, NULL);
break;
[other cases here]
}
}


Now we can start to notice redundancy. First thing would be the repetition that's actually shown here; that of "Animation_Walk_Up.Sequence[Player.currentFrame]". Applying the principle from before, we're doing a bunch of work *with* some other object; instead, let's have the work done *by* that object - I'll assume it's of type Frame, and give it its own Render method:


enum {UP, RIGHT, DOWN, LEFT} Direction;

void Frame::Render() {
// At this point, the direction has already been "processed" - it was used
// to determine which Frame to Render() - so it's not passed along.
DDback->Blt(&dest_rect, DDTile, &src_rect, DDBLT_WAIT | DDBLT_KEYSRC, NULL);
}

void Sprite::Render(Direction d) {
switch (d) {
case UP:
// Now we just need to use the local members in order to find an appropriate
// Frame and Render() it.
Animation_Walk_Up.Sequence[currentFrame].Render();
break;
[other cases here]
}
}


Now the next step is to look at that switch statement - in general, switch and if constructs are the first places to look for redundancy (or other reasons to refactor) :) Here, I assume you will end up with basically the same code for each case, except using other members Animation_Walk_Right, Animation_Walk_Down and Animation_Walk_Left. Since an enumeration is convertible to an int (but in C++, not the other way around), we can use it to index an array, which is exactly what we're going to do - physically this will be an array of four Animations indexed 0..3, but logically it will be indexed by "all possible Directions", which is to say it "holds Animation sequences for all possible Directions". Nice and neat. :) I will trust you to be able to make the other necessary changes to the Sprite declartion for this:


enum {UP, RIGHT, DOWN, LEFT} Direction;

void Frame::Render() {
// At this point, the direction has already been "processed" - it was used
// to determine which Frame to Render() - so it's not passed along.
DDback->Blt(&dest_rect, DDTile, &src_rect, DDBLT_WAIT | DDBLT_KEYSRC, NULL);
}

void Sprite::Render(Direction d) {
// Now we index an array of walk animations to figure out where to go.
Walk_Animations[d].Sequence[currentFrame].Render();
}


See how much neater things are now :) Anyway, depending on the rest of the code, we can go further with this. For example, if there will become other Sprite methods that delegate similarly to a Frame, we may want to extract a "helper" function to do the common part "(foo)_Animations[d].Sequence[currentFrame]". This might in turn involve creating another enumeration for "animation type", and collapsing all the sets of animations (walk animations, attack animations etc.) into an array:


// This would be private, since it's just used to facilitate implementation of
// "real" functionality, and is not really part of the Sprite interface.
Frame& Sprite::getFrame(AnimationType a, Direction d) {
return Animations[a][d].Sequence[currentFrame];
}

void Sprite::Render(Direction d) {
getFrame(WALK, d).Render();
}

Share this post


Link to post
Share on other sites
Alright, sounds good. I'll try to fix my code like that. Sorry for the late reply, a fix up got me banned :(

Share this post


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

  • Advertisement