Sign in to follow this  
derek7

if and refactor

Recommended Posts

I think condition is core of a code. but code with condition is hard to split to small block code. a function that have long if statement is hard to refactor. more worse,a condition usually depends on many object. forexample: void function { .... if(..) object.method(); if(..) object2.method2(); else if(...) object3.method3(); else ojbect4.method4(); ..... } it is disaster to me. are there some very good way to handle this?

Share this post


Link to post
Share on other sites
Quote:
Original post by Antheus
Post the conditions source code. This is where the refactoring will need to be done.


do you mean it has not good way to solve it, must it be solved according to every concrete spot/circs?


just for reference
it show different model and texture according menu item.any other suggestion is very welcome and desired.

int gameloop()
{
MSG msg;
::ZeroMemory(&msg, sizeof(MSG));

while(msg.message != WM_QUIT)
{
std::cout<<action<<'\n';

// message part
if(::PeekMessage(&msg, 0, 0, 0, PM_REMOVE))
{
::TranslateMessage(&msg);
::DispatchMessage(&msg);
}
// game part
else
{
static int pre_action = 0;

if ( pre_action != action )
{

switch(action)
{
case WALK:
littleMessenger.load("../media/skin_tiny.x", "tiny", &device);
break;

case JUMP:
littleMessenger.load("../media/firemouse.x", "firemouse", &device);
break;

case SUMMER:
{
D3DMATERIAL9 m = littleMessenger.skinnedmesh()->materialCurrentSet()[0]->material9();
d->SetMaterial(&m);
if(pre_action == 1)
{
d->SetTexture(0,m_summer);
}
else
{
d->SetTexture(0,t_summer);
}
}
break;

case SPRING:
{
D3DMATERIAL9 m = littleMessenger.skinnedmesh()->materialCurrentSet()[0]->material9();
d->SetMaterial(&m);
if(pre_action == 1)
{
d->SetTexture(0,m_spring);
}
else
{
d->SetTexture(0,t_spring);
}
}
break;

}
}

if(action<3)
{
pre_action = action;
}

static float lastTime = (float)timeGetTime();
static float currTime = (float)timeGetTime();
float timeDelta = (currTime - lastTime) * 0.001f;

lastTime = currTime;
currTime = (float)timeGetTime();


if (1)
{
littleMessenger.update(timeDelta);

D3DXMATRIX proj;
D3DXMatrixPerspectiveFovLH(&proj, 3.1415926*.5f, 1.3333f, 1.0f, 10000.f);
d->SetTransform(D3DTS_PROJECTION,&proj);

D3DXMATRIX campos;
D3DXMatrixLookAtLH( &campos, &D3DXVECTOR3(0,3,-155), &D3DXVECTOR3(0,0,0), &D3DXVECTOR3(0,1,0) );
d->SetTransform(D3DTS_VIEW, &campos);

d->SetRenderState(D3DRS_LIGHTING,0);
d->LightEnable(0,0);

d->Clear(0, 0, D3DCLEAR_TARGET | D3DCLEAR_ZBUFFER, 0xfff, 1.0f, 0);

d->BeginScene();

littleMessenger.render(d);

d->EndScene();

d->Present(0,0,0,0);


}
else
{
Sleep(1);
}



}
}
return msg.wParam;
}



Share this post


Link to post
Share on other sites
well, you should really separate all of those individual pieces into either a class, or at the very least separate functions to handle the different tasks.

For example, splitting all the texture stuff into a function called updateTextures( ) and have the view transform changes in a function called updateView or something like that.

Ideally, if you are using C++, I would suggest figuring out the core "pieces" of your program and make classes to represent them. That way you can encapsulate all the functionality instead of cramming it all directly in your main loop.

[edit]
I also just noticed that you have a switch statement in which you are switching not only on actions of a model, but on what season it is? This is another example of your current mess of non-related things all being crammed together. Separate and encapsulate my friend.

Share this post


Link to post
Share on other sites



static int pre_action = 0;

void handleMovement(int action)
{
switch(action)
{
case WALK:
littleMessenger.load("../media/skin_tiny.x", "tiny", &device);
break;

case JUMP:
littleMessenger.load("../media/firemouse.x", "firemouse", &device);
break;
}
}

D3DMATERIAL9 getMaterialForSeason(int oldSeason, int newSeason)
{
switch(newSeason)
{
case SUMMER : return (oldSeason == 1) ? m_summer : t_summer;
case SPRING : return (oldSeason == 1) ? m_spring : t_spring;
}
return NULL;
}

void handleSeasonChange(int oldSeason, int newSeason)
{
D3DMATERIAL9 m = littleMessenger.skinnedmesh()->materialCurrentSet()[0]->material9();
d->SetMaterial(&m);
d->SetTexture(0, getMaterialForSeason(oldSeason, newSeason);
}

void handleAction(int newAction)
{
if ( pre_action != action )
{

switch(action)
{
case WALK:
case JUMP:
handleMovement(action);
break;

case SUMMER:
case SPRING:
handleSeasonChange(pre_action, action);
break;
}
}
if ( action < 3 ) pre_action = action;
}

void doTimeStep(float timeDelta)
{
if (1)
{
littleMessenger.update(timeDelta);

D3DXMATRIX proj;
D3DXMatrixPerspectiveFovLH(&proj, 3.1415926*.5f, 1.3333f, 1.0f, 10000.f);
d->SetTransform(D3DTS_PROJECTION,&proj);

D3DXMATRIX campos;
D3DXMatrixLookAtLH( &campos, &D3DXVECTOR3(0,3,-155), &D3DXVECTOR3(0,0,0), &D3DXVECTOR3(0,1,0) );
d->SetTransform(D3DTS_VIEW, &campos);

d->SetRenderState(D3DRS_LIGHTING,0);
d->LightEnable(0,0);

d->Clear(0, 0, D3DCLEAR_TARGET | D3DCLEAR_ZBUFFER, 0xfff, 1.0f, 0);

d->BeginScene();

littleMessenger.render(d);

d->EndScene();

d->Present(0,0,0,0);
}
else
{
Sleep(1);
}
}

static float lastTime = (float)timeGetTime();
static float currTime = (float)timeGetTime();

int gameloop()
{
MSG msg;
::ZeroMemory(&msg, sizeof(MSG));

while(msg.message != WM_QUIT)
{
std::cout<<action<<'\n';

// message part
if(::PeekMessage(&msg, 0, 0, 0, PM_REMOVE))
{
::TranslateMessage(&msg);
::DispatchMessage(&msg);
}
// game part
else
{
handleAction(action);

float timeDelta = (currTime - lastTime) * 0.001f;
lastTime = currTime;
currTime = (float)timeGetTime();

doTimeStep(timeDelta);

}
}
return msg.wParam;
}



This is the refactored code, according to functionality. After you separate the logic into functional units, you can focus on improving individual pieces (getMaterialForSeason, handleMovement, doTimeStep).

Next step would be also, to separate "action" into movementAction and worldAction, where first handles the JUMP and WALK, and worldAction handles season changes. This would simplify the corresponding methods.

Share this post


Link to post
Share on other sites
At the very least, separate out the control, world updates, and display.

This is *NOT* refactored code, because it breaks what you had in place.

But that's probably a good thing.


DoProcessing( MSG &msg ) {
timestamp = GetRealWorldTime();
ProcessMessage(msg);
UpdateWorldState();
DoGraphics();
...
}

ProcessMessage( MSG &msg ) {
...
switch( msg.whatever ) {
case foo: this.player.move( P_LEFT ); break;
case bar: this.player.jump(); break;
...
}
...
}

UpdateWorldState() {
if( game.IsPaused() ) {
lastWorldTimestamp = timestamp;
return;
}
if( lastWorldStateTimestamp + MOTION_DELTA > timestamp )
// No processing
return;
if( lastWorldStateTimestamp + MAX_MOTION_DELTA < timestamp ) {
/* Last frame took too long. We might be in a debugger or
have some other issues. Do something intellegent.
*/

...
}
else {
/* Advance a single animation frame */
lastWorldStateTimestamp += MOTION_DELTA;
...
/* Do animation stuff */
...
}

etc.


Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this