You may notice, if you've read through my code at all, that I like short concise methods. The names are also fairly descriptive, generally telling the exact purpose of the method. Yet, I encounter code quite frequently that make me want to do harm unto some people. Recently I encountered one such block of code, and so I think I shall detail exactly what I feel is wrong with this particular piece of code (which you will find in listing 1 at the end of this post).

First things first, what do I consider a well written method? A method which has a singular purpose that is expressed clearly by the code that it contains. Note the bold, this is not coincidental. The singular purpose obviously this means that the method should not do several distinctly different things. For instance, if you look at listing 2 you will see my example of a method which does far to many things. The other requirement, expressed clearly by the code, is a harder one to quantify. Primarily it involves writing methods which are to the point, and often composed of other smaller methods that perform sub jobs. However, you will note that I didn't say that it should be short. A method can be long, as long as it meets the above requirements.

So, how can you tell if your code is doing to many things? Simple, define it's behavior as a sentence. The code shown in listing 1 could be written as: "A function which will create a window, then it will create a Direct3d object, and finally it will create a Direct3D device." Notice the then's, those are the key to a function that does too many things. Obviously, this function should be split into three parts, at the very least. The function in listing 2 is impossible to quantify, and should just be thrown out entirely [grin].

As for code readability, you can clearly see that the code in listing 1 is not clear. It does far to many things at once, has variables mixed hither and thither, and generally doesn't flow in a clear and readable manner. Of course, extracting the three purposes of this function out to their own methods will help, but then you will need to further refactor those methods so that they too have a singular purpose that is expressed clearly by the code.

So, what kinds of refactorings could we do on the code in listing 1? Well, first of all, we could extract the three parts into their own methods, each with names that clearly express their intent. Examples of appropriate names would be: CreateWindow, CreateDirect3D, CreateDevice. Then once you have those extracted, you could figure out sentances explaining what they themselves do, for instance the CreateWindow method would be something like: "Create and register a window class, then create a window." Hm, I see a then in there! Don't be too esoteric in defining these sentences, for you will end up with a bunch of one line methods that do basically nothing at all. However, this is clearly not such a case. So by extracting the class registration out to another method (and replacing it with the appropriate method call), we have reduced the complexity of the code and made it easier to read. Another particular tidbit I would add would be exception handling. Returning false each time and checking the results just isn't needed. Proper use of the CComPtr class, along with throwing exceptions on errors, can make your code both cleaner and safer.

Exceptions are a godsend. They have been given unto you to simplify your error reporting and simplify error detection. Writing code that is exception safe isn't particularly hard to do either, it just requires a little bit of forethought. So why is it that I don't see more games using them? Well, one of the rather pathetic excuses I've heard was: They are slow. Oh how I laughed. You are going to complain about speed when you've just encountered an exceptional condition? I mean honestly. If you're throwing an exception for a non-critical (for that particular module) error, then you need help. If on the other hand you have just encountered a situation that leaves the object in an unknown state, throw an exception. One area where exceptions are useful comes in the form of file handling. You open a file, it's not there, your file handler throws a FileNotFound exception. Or perhaps the file is there, but you don't have permissions to access it. This is clearly a case where the file handler will be in an unknown state, so throwing an exception was the correct thing to do. However, consider a different case... You are traversing your collision graph, and try and collide an object marked as dead against another. Since dead objects are no longer considered collidable, you throw an exception. But this is a case where throwing an exception is just plain stupid. There could be all sorts of reasons why the dead object is still in the collision graph, for instance, perhaps you wait till the end to clean up dead objects. It would be better to simply return a false.

Clearly the code in listing 1 could use exceptions. In fact, every error there could be cleaning dealt with just by the addition of two CComPtr's and a couple of throws. Not only would it be cleaner (no more checking the results of each failed and then returning a failure and then checking for failure...etc), but it would also be safer. Code that uses exceptions clearly indicates error conditions. What does a return value tell you? Not a heck of a lot. It's only by viewing the code in the context of its usage that the return value becomes meaningful. On the other hand, an exception will indicate an error condition unambiguously.

Finally, lets look at the name of the method, InitD3D. Is that what it really does? Because, if I'm not mistaken, that's what our CreateDirect3D and/or CreateDevice methods do. So what does this method do? Well, it creates a window, then it creates a direct 3d object, then it creates a device. So, eliminate this method entirely, and expose the three methods we wrote above to the callee.

Of course, if you must know where I got this piece of code, it was from a paste that one beginner made. He copied the code from a book in order to learn how to code DirectX 9. I pity that poor newbie. This book, which is supposed to be an introductory book, introduces newbies to code that, quite frankly, is not production quality. If I ever saw a method even close to this in a production environment, you would be called to task in the next code review, or even before that. So why is such crap allowed to be published? Simple economics: The writer is a newbie himself, and the publish knows that there are thousands, even hundreds of thousands, of eager newbies waiting to eagerly read this drivel in the hopes that they will learn to write games.

Quote:Listing 1

bool d3d::InitD3D(    HINSTANCE hInstance,    int width, int height,    bool windowed,    D3DDEVTYPE deviceType,    IDirect3DDevice9** device){    //    // Create the main application window.    //    WNDCLASS wc;    wc.style         = CS_HREDRAW | CS_VREDRAW;    wc.lpfnWndProc   = (WNDPROC)d3d::WndProc;     wc.cbClsExtra    = 0;    wc.cbWndExtra    = 0;    wc.hInstance     = hInstance;    wc.hIcon         = LoadIcon(0, IDI_APPLICATION);    wc.hCursor       = LoadCursor(0, IDC_ARROW);    wc.hbrBackground = (HBRUSH)GetStockObject(WHITE_BRUSH);    wc.lpszMenuName  = 0;    wc.lpszClassName = "Direct3D9App";    if( !RegisterClass(&wc) )     {        ::MessageBox(0, "RegisterClass() - FAILED", 0, 0);        return false;    }            HWND hwnd = 0;    hwnd = ::CreateWindow("Direct3D9App", "Direct3D9App",         WS_EX_TOPMOST,        0, 0, width, height,        0 /*parent hwnd*/, 0 /* menu */, hInstance, 0 /*extra*/);     if( !hwnd )    {        ::MessageBox(0, "CreateWindow() - FAILED", 0, 0);        return false;    }    ::ShowWindow(hwnd, SW_SHOW);    ::UpdateWindow(hwnd);    //    // Init D3D:     //    HRESULT hr = 0;    // Step 1: Create the IDirect3D9 object.    IDirect3D9* d3d9 = 0;    d3d9 = Direct3DCreate9(D3D_SDK_VERSION);    if( !d3d9 )    {        ::MessageBox(0, "Direct3DCreate9() - FAILED", 0, 0);        return false;    }    // Step 2: Check for hardware vp.    D3DCAPS9 caps;    d3d9->GetDeviceCaps(D3DADAPTER_DEFAULT, deviceType, &caps);    int vp = 0;    if( caps.DevCaps & D3DDEVCAPS_HWTRANSFORMANDLIGHT )        vp = D3DCREATE_HARDWARE_VERTEXPROCESSING;    else        vp = D3DCREATE_SOFTWARE_VERTEXPROCESSING;    // Step 3: Fill out the D3DPRESENT_PARAMETERS structure.     D3DPRESENT_PARAMETERS d3dpp;    d3dpp.BackBufferWidth            = width;    d3dpp.BackBufferHeight           = height;    d3dpp.BackBufferFormat           = D3DFMT_A8R8G8B8;    d3dpp.BackBufferCount            = 1;    d3dpp.MultiSampleType            = D3DMULTISAMPLE_NONE;    d3dpp.MultiSampleQuality         = 0;    d3dpp.SwapEffect                 = D3DSWAPEFFECT_DISCARD;     d3dpp.hDeviceWindow              = hwnd;    d3dpp.Windowed                   = windowed;    d3dpp.EnableAutoDepthStencil     = true;     d3dpp.AutoDepthStencilFormat     = D3DFMT_D24S8;    d3dpp.Flags                      = 0;    d3dpp.FullScreen_RefreshRateInHz = D3DPRESENT_RATE_DEFAULT;    d3dpp.PresentationInterval       = D3DPRESENT_INTERVAL_IMMEDIATE;    // Step 4: Create the device.    hr = d3d9->CreateDevice(        D3DADAPTER_DEFAULT, // primary adapter        deviceType,         // device type        hwnd,               // window associated with device        vp,                 // vertex processing        &d3dpp,             // present parameters        device);            // return created device    if( FAILED(hr) )    {        // try again using a 16-bit depth buffer        d3dpp.AutoDepthStencilFormat = D3DFMT_D16;                hr = d3d9->CreateDevice(            D3DADAPTER_DEFAULT,            deviceType,            hwnd,            vp,            &d3dpp,            device);        if( FAILED(hr) )        {            d3d9->Release(); // done with d3d9 object            ::MessageBox(0, "CreateDevice() - FAILED", 0, 0);            return false;        }    }    d3d9->Release(); // done with d3d9 object        return true;}

Quote:Listing 2

double f(double d,int a = -1,int b = 10) {	return a>1?d?d*f(d-1,a,b):1:		a==1?b?d*f(d,a,b-1):1:		!a?d>=0?d:-d:b?f(-1,1,b-1)/f(2*b-1,2,0)*f(d,1,2*b-1)+f(d,a,b-1):0;}