# [C++] Need help on optimizing a function

## Recommended Posts

Hi. There is a function in my mouse input handler that has always bothered me, and now that the whole manager is done, I want to optimize it a bit.
	if (input->header.dwType == RIM_TYPEMOUSE)
{

// left mouse button
if(input->data.mouse.usButtonFlags & RI_MOUSE_LEFT_BUTTON_DOWN)
{
in_event.button = 1 | mouse_offset;
in_event.buttonstate = 1;
update(in_event);
}
else if(input->data.mouse.usButtonFlags & RI_MOUSE_LEFT_BUTTON_UP)
{
in_event.button = 1 | mouse_offset;
in_event.buttonstate = 0;
update(in_event);
}

// Middle mouse button
if(input->data.mouse.usButtonFlags & RI_MOUSE_MIDDLE_BUTTON_DOWN)
{
in_event.button = 2 | mouse_offset;
in_event.buttonstate = 1;
update(in_event);
}
else if(input->data.mouse.usButtonFlags & RI_MOUSE_MIDDLE_BUTTON_UP)

{
in_event.button = 2 | mouse_offset;
in_event.buttonstate = 0;
update(in_event);
}

// Right mouse button
if(input->data.mouse.usButtonFlags & RI_MOUSE_RIGHT_BUTTON_DOWN)
{
in_event.button = 3 | mouse_offset;
in_event.buttonstate = 1;
update(in_event);
}
else if(input->data.mouse.usButtonFlags & RI_MOUSE_RIGHT_BUTTON_UP)
{
in_event.button = 3 | mouse_offset;
in_event.buttonstate = 0;
update(in_event);

}
// Mouse movement
if(input->data.mouse.lLastX)
{
in_event.mouse_x = input->data.mouse.lLastX;
}
if(input->data.mouse.lLastY)
{
in_event.mouse_y = input->data.mouse.lLastY;
}
if (in_event.mouse_x || in_event.mouse_y)
{
update(in_event);
}

// Mouse button 4
if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_4_DOWN)
{
in_event.button = 4 | mouse_offset;
in_event.buttonstate = 1;
update(in_event);
}
else if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_4_UP)
{	in_event.button = 4 | mouse_offset;
in_event.buttonstate = 0;
update(in_event);
}

// Mouse button 5
if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_5_DOWN)
{	in_event.button = 5 | mouse_offset;
in_event.buttonstate = 1;
update(in_event);
}
else if(input->data.mouse.usButtonFlags & RI_MOUSE_BUTTON_5_UP)
{	in_event.button = 5 | mouse_offset;
in_event.buttonstate = 0;
update(in_event);

}

if(input->data.mouse.usButtonFlags & RI_MOUSE_WHEEL)
{
HandleMouseWheel(input);
}
return;

}


It reeks of some nasty things, like code duplication and spaghetti else-if-heimer, however I can't really think of too many optimizations, since the class is setting values depending on other values, and it has to be reasonably quick, so I'm not sure if a generic way of doing it is possible, but who knows? Profiling showed that in the current state, the function is fast enough, but I still don't like my implementation, and I want to improve my style. So, can I rewrite this to reduce code duplication, while keeping the amount of calls as low as possible? Edit: What I've been thinking of so far: -Those "offset" calculations will be replaced by #defines -more returns -extracting each section into a respective inline function for readability -Rearrange more stuff, but that's the vague part :P

##### Share on other sites
For starters, performance isn't a concern in a function that's called once per frame.

About improving duplicate code, you can
a) refactor common stuff into a function
b) use a table for the data (bitmasks etc.) of the different mouse buttons and loop through it

##### Share on other sites
Of course, a loop makes sense, seeing how the tests are pretty much done sequentially, d'oh, I'm blind!

And well, I suppose a one frame function can slack a little...hm.

Cheers~!

##### Share on other sites
At the very least, you can turn all those sections for different buttons into a loop, by using an auxiliary array that contains what you need to know about each button. Your code would then look something like this:
    if (input->header.dwType == RIM_TYPEMOUSE) 	{		// Buttons		for (int b = 0; b < number_of_buttons; ++b)		{			ButtonDescription const &bd = button_description[b];			if (input->data.mouse.usButtonFlags & bd.button_down)			{				in_event.button = bd.code | mouse_offset;				in_event.buttonstate = 1;				update(in_event);			}			else if(input->data.mouse.usButtonFlags & bd.button_up)			{  				in_event.button = bd.code | mouse_offset;				in_event.buttonstate = 0;				update(in_event);			}		}		// Mouse movement		if(input->data.mouse.lLastX)		{			in_event.mouse_x = input->data.mouse.lLastX;		}		if(input->data.mouse.lLastY)		{			in_event.mouse_y = input->data.mouse.lLastY;		}		if (in_event.mouse_x || in_event.mouse_y)		{			update(in_event);		}				// Wheel		if(input->data.mouse.usButtonFlags & RI_MOUSE_WHEEL)		{			HandleMouseWheel(input);		}		return;		}

Also, you shouldn't avoid function calls in general. Performance is not as bad as you think (not bad at all if they can be inlined), and most of the time performance is not a good reason to not do something that makes your code more clear anyway.

Stay away from macros (#define).

##### Share on other sites
Thanks for the reply, yeah, I turned the spaghetti into a loop and the values in a table.

DWORD button = input->data.mouse.usButtonFlags;for (int i = 0; i < 10; ++i  )    {	        in_event.buttonstate = !in_event.buttonstate;        if (button & mousestates[i])	{	in_event.button = mousebuttons[i];	update(in_event);	}    }

Looks like this, now.

Since the button down and up checks were done in an alternating fashion, I'm letting the buttonstate alternate as well, should work as intended, I hope. Hehe.

And again, thanks for the pointers about performance concerns, I suppose it's because I'm learning it mainly on my own, and thus I am usually considering the worst case first :P.

Oh, and I pretty much never use self made macros of any kind, but I figured naming the constant while experimenting would make things more manageable. Hehe.

Well, cheers!

##### Share on other sites
Quote:
 Original post by EriusAnd again, thanks for the pointers about performance concerns, I suppose it's because I'm learning it mainly on my own, and thus I am usually considering the worst case first :P.

Yeah, premature optimization can make one paranoid. I'm guilty of that as well (and micro-optimization is a habit that's hard to get rid of).

##### Share on other sites
Naming constants is perfectly fine, but that should be done using const', not #define'.

  unsigned const mouse_offset = ...

##### Share on other sites
Writing less code is usually a good idea, and in most cases it will even improve performance. You get good results if you split your code up into functions, but reference the same function as few times as possible. Your revised code calls update(in_event) in a loop, and it will be called just as many times as before, but now the compiler only needs to produce one copy of the register push instructions, so your object code is smaller. And the benefit is magnified if the compiler decides to inline the update function.

## Create an account

Register a new account

• ### Forum Statistics

• Total Topics
628379
• Total Posts
2982353

• 10
• 9
• 15
• 24
• 11