• Advertisement
  • entries
    146
  • comments
    436
  • views
    198317

Code clarity

Sign in to follow this  

277 views

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;
}
Sign in to follow this  


7 Comments


Recommended Comments

Wow, you really scared the crap out of me for a second there, thought it was my code for a second!

Then I saw how bad it really is. In all fairness, I do stuff the initialization calls for both D3D and the D3DDevice into the same function, because, for my use, I'd never use one without the other.

That given, throwing in the registration and creation of a window is completely stupid. IMO, anything that can be logically contained within its own class SHOULD be contained within its own class. Windows are no exception.

Personally, I have two completely different coding styles: one for my personal code, and one for code other people might use. Whenever I code for other people, I make sure to make it all nice and pretty, with those big comments that list out what each function does and such. But I can remember what each function does, even from code I wrote six months ago. And I know you're going to comment how one should always practice good coding skills to prevent the development of bad habits, but peh.

At the very least, I openly admit I'm a lousy coder [wink]
Who, funnily enough, runs a lousier OS. ++score;

Share this comment


Link to comment
Quote:
Wow, you really scared the crap out of me for a second there, thought it was my code for a second!

It is your code, I just made it less bad than yours [grin]
Quote:

Then I saw how bad it really is. In all fairness, I do stuff the initialization calls for both D3D and the D3DDevice into the same function, because, for my use, I'd never use one without the other.

They can be in the same function, a function which calls the two functions that I suggested refactoring out.
Quote:

That given, throwing in the registration and creation of a window is completely stupid. IMO, anything that can be logically contained within its own class SHOULD be contained within its own class. Windows are no exception.

Of course it should, heck even something that you would expect to be a function could be a function object if it will simplify the code.
Quote:

Personally, I have two completely different coding styles: one for my personal code, and one for code other people might use. Whenever I code for other people, I make sure to make it all nice and pretty, with those big comments that list out what each function does and such. But I can remember what each function does, even from code I wrote six months ago. And I know you're going to comment how one should always practice good coding skills to prevent the development of bad habits, but peh.

One should never slack off from proper coding, even when in the silence of your mind.
Quote:

At the very least, I openly admit I'm a lousy coder [wink]
Who, funnily enough, runs a lousier OS. ++score;

So I've noticed.

Share this comment


Link to comment
I have come to love exceptions, but the mere generation of exception handling code has traditionally introduced performance penalties (eg. larger code size, optimisations eliminated due to the added possible branching, etc). However many such comparisons were done on VC6 which didn't properly support marking a function as throwing no exceptions. Perhaps in more recent compilers it's possible to use exceptions everywhere except the performance-critical parts of the main loop, getting the best of both worlds.

Share this comment


Link to comment
lol it's actually really funny because today at work someone was talking about how expensive exceptions are to throw in C# and .NET languages in general.

I thought about it for a second and basically said... "Do you plan on throwing exceptions alot? .. because that worries me" :)

I love exceptions especially in C#.. they have saved me hours of debugging work. I must admit that when using C++ I slack a lot with exceptions.. generally because I never used them until VS2002 unless I was working with a very large project.

Share this comment


Link to comment
Quote:
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.

I'm not quite convinced on this one. Basically, what you've been talking about here is cohesion. Functional cohesion, having one and only purpose per function, is the target. However, this doesn't work with all routines. Some routines, like startup and shutdown routines, typically have temporal and sequential cohesion - they have to do various tasks, in a specific order, one after another. In that case, the best thing to do is just make sure the routine calls other functionally-cohesive routines to do its work, instead of doing it itself.

So, exposing the 3 methods we wrote to the above callee will simply make the callee the temporally and sequentially cohesive routine - I believe that an equivalent solution would've been to just rename InitD3D to Direct3DStartup, or GraphicsStartup, and you're done.

Actually, a more generic approach would be to take window creation entirely outside the d3d class/namespace. It should go in its own separate module, and d3d device initialization should be parameterized with a window handle. The caller of these should be some GraphicsStartup() or Direct3DStartup() routine, at a higher-level. This allows for operating on the window handle for further support of windowed-mode scenarios.

Quote:
I must say Washu, your journal posting quality is enviable.

Indeed!
Washu, I salute thee with respect and honour - You set a standard. I owe you a lot! (That craftsman series for one [smile])

Keep it up.

Share this comment


Link to comment
Quote:

Quote:
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.

I'm not quite convinced on this one. Basically, what you've been talking about here is cohesion. Functional cohesion, having one and only purpose per function, is the target. However, this doesn't work with all routines. Some routines, like startup and shutdown routines, typically have temporal and sequential cohesion - they have to do various tasks, in a specific order, one after another. In that case, the best thing to do is just make sure the routine calls other functionally-cohesive routines to do its work, instead of doing it itself.

Oh, I'm not saying that you shouldn't provide a single method which calls both of the initialization methods (the Direct3D ones). Especially since creating the D3D object is generally followed by creating the Device.
Quote:

So, exposing the 3 methods we wrote to the above callee will simply make the callee the temporally and sequentially cohesive routine - I believe that an equivalent solution would've been to just rename InitD3D to Direct3DStartup, or GraphicsStartup, and you're done.

Actually, if it was me, I would remove the windowing stuff entirely, it really deserves it's own class, along with the message handling (which was in the d3d class AS WELL). Basically, this class should deal with D3D and that's it. Although I didn't state as much directly, I was trying to abstractly infer this.
Quote:

Actually, a more generic approach would be to take window creation entirely outside the d3d class/namespace. It should go in its own separate module, and d3d device initialization should be parameterized with a window handle. The caller of these should be some GraphicsStartup() or Direct3DStartup() routine, at a higher-level. This allows for operating on the window handle for further support of windowed-mode scenarios.

See above [grin], good to know that we are on the same page.

Share this comment


Link to comment

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

  • Advertisement