Optimizing my c++ code

Started by
26 comments, last by visitor 15 years, 4 months ago
Yikes lots of info, lots of different viewpoints... I feel I am biting more than I can chew.

Do you guys think it would be a bad idea to go with the info I've gathered, the bits and pieces from here and there and work with that? Try and learn the rest as I go?
Advertisement
Learning "proper" OOP is not so easy as it looks. But it can considerably elevate the code abstraction level:

  • The most important gain from using objects is to encapsulate data and algorithms behind an interface (an object). That way you can change underlying implementations (mostly done via private members or other means to hide away messy details) without having to change all code that is using the object.
  • Second, you can re-use classes via subclassing.
  • Third, OOP then enables complex forms of abstraction via use of interfaces (you can exchange classes and thus implementations in your code when these classes have common interfaces, possibly during runtime if you're using virtual methods).

    You'll have to bite the bullet and experiment with OOP, to see how you can use it with your code. A good way to learn fast is to use OOP code, from a premade engine or another software (I learned lots about OOP from Qt GUI framework).
  • Quote:Original post by DevFred
    Quote:Original post by godsenddeath
    The only "loose" functions you should have would be utility functions

    Haven't you read How Non-Member Functions Improve Encapsulation?

    I do not buy into this. Artificially breaking the public interface into member and non-member functions feels unnatural and just serves to show off the "minimality" of the class interface. At some point, as additional requirements surface, some non-members will have to be integrated back into the class, making the free functions redundant, or the public interface will have to be extended to cover that additional functionality, thus showing how unneccessary the separation was in the first place.
    Hmm.. so I decided to try and get down to it.. and couldn't I really don't know how to get down with it.

    For example this is the code I have for creating a window and initilizing everything else.. I don't know hot to make it more oriented programish or implement encapsulation.. :S

    How would could I do it just for example?

    #include "global.h"input inputData;int WINAPI WinMain(HINSTANCE hInstance,                   HINSTANCE hPrevInstance,                   LPSTR lpCmdLine,                   int nCmdShow){    HWND hWnd;	if(!displayWindow(&hWnd, hInstance, nCmdShow))		return 0;	if(!initD3D(&hWnd))				return 0;	loadGraphics();	mainLoop();    closeDX();    return 0;}bool displayWindow (HWND *hWnd, HINSTANCE hInstance, int nCmdShow){	WNDCLASSEX wc;    ZeroMemory(&wc, sizeof(WNDCLASSEX));	wc.cbSize = sizeof(WNDCLASSEX);	wc.style = CS_HREDRAW | CS_VREDRAW;	wc.lpfnWndProc = (WNDPROC)WindowProc;	wc.hInstance = hInstance;	wc.hCursor = LoadCursor(NULL, IDC_ARROW);	wc.lpszClassName = "WindowClass1";    if(RegisterClassEx(&wc)==0)		return FALSE;    *hWnd = CreateWindowEx(NULL,                          "WindowClass1",                          "Dungeon",						  WS_OVERLAPPEDWINDOW,//						  WS_EX_TOPMOST | WS_POPUP,                          0, 0,                          SCREEN_WIDTH, SCREEN_HEIGHT,                          NULL,                          NULL,                          hInstance,                          NULL);	if(*hWnd == NULL)		return FALSE;    ShowWindow(*hWnd, nCmdShow);	return TRUE;}bool handleMessages(){	static MSG msg;	if(PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)){		if(msg.message == WM_QUIT)			return FALSE;		TranslateMessage(&msg);		DispatchMessage(&msg);	}	return TRUE;}LRESULT CALLBACK WindowProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam){    switch(message)	{        case WM_DESTROY:			{                PostQuitMessage(0);                return 0;            } break;		case WM_KEYDOWN:			{				switch(wParam)				{				case VK_RIGHT:					{						inputData.turn_right = true;					} break;									case VK_LEFT:					{						inputData.turn_left = true;					} break;				case VK_UP:					{						inputData.move_forward = true;					} break;				case VK_DOWN:					{						inputData.move_backward = true;					} break;				case VK_SPACE:					{						inputData.shooting = true;					} break;				case VK_ESCAPE:					{						DestroyWindow(hWnd);					} break;				}			} break;		case WM_KEYUP:			{				switch(wParam)				{				case VK_RIGHT:					{						inputData.turn_right = false;					} break;									case VK_LEFT:					{						inputData.turn_left = false;					} break;				case VK_UP:					{						inputData.move_forward = false;					} break;				case VK_DOWN:					{						inputData.move_backward = false;					} break;				case VK_SPACE:					{						inputData.shooting = false;					} break;				}			} break;	}    return DefWindowProc (hWnd, message, wParam, lParam);}
    GD.net has a good article on this very subject.

    But to try to answer your question:
    The first thing will be to get these free functions into a class, so that you can just do:
    Window window;if(!window.displayWindow(hInstance, nCmdShow))    return 0;if(!initD3D(window.getHandle()))    return 0;



    You can then also create a D3D object that just takes a window as a parameter, so you can do
    Window window;if(!window.display(hInstance, nCmdShow))    return 0;D3D d3d(window);if(!d3d.init())    return 0;


    Notice that you're not passing the HWND directly. It's "encapsulated" by the Window object.


    What you're doing when you move code around is called "refactoring", and it's pretty easy to do when you're used to it. There's a page about refactoring and you should get one of the recommended books if you're serious about software engineering.

    Quote:
    I do not buy into this. Artificially breaking the public interface into member and non-member functions feels unnatural and just serves to show off the "minimality" of the class interface. At some point, as additional requirements surface, some non-members will have to be integrated back into the class, making the free functions redundant, or the public interface will have to be extended to cover that additional functionality, thus showing how unneccessary the separation was in the first place.


    Both sides have good arguments. In any case it is important to avoid reduplicating code and to implement things in terms of other things where possible. There is nothing in member functions that says they cannot or shouldn't be implemented in terms of other (public) methods (except you are not forced to).
    Alright I think I am getting something..

    Here's the header with the new class I created for it and the new source file..
    Although I am getting some stranger errors I don't get..

    1>c:\documents and settings\david\mis documentos\visual studio 2005\projects\dungeon\dungeon\window.h(30) : error C2533: 'application::{ctor}' : constructors not allowed a return type
    1>c:\documents and settings\david\mis documentos\visual studio 2005\projects\dungeon\dungeon\window.cpp(11) : error C2264: 'application::application' : error in function definition or declaration; function not called

    header
    #include <windows.h>#include <windowsx.h>class application{private:	HWND hWnd;	WNDCLASSEX wc;	LPCSTR className;	LPCSTR windowName;	DWORD windowStyle;	int screenWidth;	int screenHeight;	static MSG msg;public:	application();	HWND getHandle();	bool displayWindow(HINSTANCE, int);	bool handleMessages();}application::application(){		className = "WindowClass1";		windowName = "Dungeon";		windowStyle = WS_OVERLAPPEDWINDOW;		screenWidth = 640;		screenHeight = 480;}HWND application::getHandle(){	return hWnd;}bool application::displayWindow(HINSTANCE hInstance, int nCmdShow){    ZeroMemory(&wc, sizeof(WNDCLASSEX));	wc.cbSize = sizeof(WNDCLASSEX);	wc.style = CS_HREDRAW | CS_VREDRAW;	wc.lpfnWndProc = (WNDPROC)WindowProc;	wc.hInstance = hInstance;	wc.hCursor = LoadCursor(NULL, IDC_ARROW);	wc.lpszClassName = className;    if(RegisterClassEx(&wc)==0)		return FALSE;    hWnd = CreateWindowEx(NULL,                          className,                          windowName,						  windowStyle,                          0, 0,                          screenWidth,						  screenHeight,                          NULL,                          NULL,                          hInstance,                          NULL);	if(hWnd == NULL)		return FALSE;    ShowWindow(hWnd, nCmdShow);	return TRUE;}bool application::handleMessages(){	if(PeekMessage(&msg, NULL, 0, 0, PM_REMOVE)){		if(msg.message == WM_QUIT)			return FALSE;		TranslateMessage(&msg);		DispatchMessage(&msg);	}	return TRUE;}

    source
    #include "global.h"#include "window.h"input inputData;int WINAPI WinMain(HINSTANCE hInstance,                   HINSTANCE hPrevInstance,                   LPSTR lpCmdLine,                   int nCmdShow){	application app;	if(!app.displayWindow(hInstance, nCmdShow))		return 0;	if(!initD3D(app.getHandle()))		return 0;	loadGraphics();	mainLoop();    closeDX();    return 0;}LRESULT CALLBACK WindowProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam){    switch(message)	{        case WM_DESTROY:			{                PostQuitMessage(0);                return 0;            } break;		case WM_KEYDOWN:			{				switch(wParam)				{				case VK_RIGHT:					{						inputData.turn_right = true;					} break;									case VK_LEFT:					{						inputData.turn_left = true;					} break;				case VK_UP:					{						inputData.move_forward = true;					} break;				case VK_DOWN:					{						inputData.move_backward = true;					} break;				case VK_SPACE:					{						inputData.shooting = true;					} break;				case VK_ESCAPE:					{						DestroyWindow(hWnd);					} break;				}			} break;		case WM_KEYUP:			{				switch(wParam)				{				case VK_RIGHT:					{						inputData.turn_right = false;					} break;									case VK_LEFT:					{						inputData.turn_left = false;					} break;				case VK_UP:					{						inputData.move_forward = false;					} break;				case VK_DOWN:					{						inputData.move_backward = false;					} break;				case VK_SPACE:					{						inputData.shooting = false;					} break;				}			} break;	}    return DefWindowProc (hWnd, message, wParam, lParam);}


    A couple of unrelated questions..
    What does static mean?
    Is using typedefs okey?
    When/how does WindowProc get called?
    Quote:
    What does static mean?


    In front of a class member it means that this is not an instance member, but something that all instances of this class share.

    Quote:
    Is using typedefs okey?


    Absolutely.

    Unless they are more confusing than the original type, e.g typedeffing pointers is somewhat bad (though WINAPI uses it everywhere).

    typedef int* PData;...int main(){   PData dat;    ...}


    When you are not careful and things are far from each other, it is easy to think that dat is a normal default-constructed object (and therefore doesn't need to be initialized). However, it is actually just an uninitialized pointer.


    Quote:
    When/how does WindowProc get called?


    From handleMessages (and in particular as a callback by DispatchMessage). And the procedure was registered first with the window in displayWindow.

    You just need to study WinAPI references (and introductory tutorials).
    You may also want to check out the book large Scale C++ Software Design

    [size="3"]Halfway down the trail to Hell...
    You should aim to have as few members as possible. For example, your MSG variable can be local to handleMessages() as it is never used elsewhere.

    In addition your WNDCLASSEX and className variables should be local to displayWindow(), they don't appear to be used elsewhere. Your windowstyle variable is effectively a constant, so I would define it as a const local inside displayWindow(), unless you want to provide a way to change or modify this.

    Arguably the same could apply to the windowName, screenWidth and screenHeight, but I think having these values as an arguments to the constructor would be a good idea (then you can easily reuse this class in different projects). You can keep a default constructor for quick prototyping projects.

    Finally, I believe it is very poor style to cast the WNDPROC, because this could cause errors. Your WindowProc must match the declaration exactly, so a cast should be unnecessary. I would take the WindowProc as an argument to displayWindow. Currently you must be forward declaring the WindowProc somewhere. But I don't really know if that is a great idea - I don't program with raw WinAPI calls.

    This topic is closed to new replies.

    Advertisement