Critique my C++.

Started by
17 comments, last by Spoonbender 15 years, 8 months ago
Quote:source line 248: You are fopening a file with "rt", "t" is not a valid option, I think you mean just "r". I'm actually not sure if fread can even read text, it's been a long time since I've worked with windows.

Text mode ("t") is a valid option, and it is the default for POSIX-compliant file I/O if you do not specify "b" for binary mode. It just so happens that fread() ignores the text/binary flags because it is assumed to be used only for reading binary data or data of a known and fixed length.

Microsoft and Apple operating systems both need to pay attention to text versus binary modes, not just Windows. Under Mac OS, CR is used as a line separator, not LF.
RIP GameDev.net: launched 2 unusably-broken forum engines in as many years, and now has ceased operating as a forum at all, happy to remain naught but an advertising platform with an attached social media presense, headed by a staff who by their own admission have no idea what their userbase wants or expects.Here's to the good times; shame they exist in the past.
Advertisement
Historical note: that was Classic Mac OS. OS X supports of both, but AFAIK by default follows the Unix convention of LF. Windows is probably the only place CRLF differences matter anymore.

Then again, I haven't written any code for OS X in a long time, I could be wrong, but it seems like the right idea.
Your class has a non-trivial destructor, but no copy constructor or assignment operator; your class will cause a resource leak when it's copied. Rule of Three.

That's the only bug I can see - everything else is stylistic.

[Edited by - Nitage on August 12, 2008 3:26:15 AM]
Lakos (Large Scale C++) recommended warts like 'ix' to group modules/services together even when using namespaces.
I never followed that advice myself though.

Consider using int32_t, uint32_t, et. al. instead of uint32. The compiler might even have the C99 headers stdint.h & stdbool.h with them defined already.
Hate the _t's myself but use 'em anyway.

line 12: int PalAddress;
If it's an address why isn't it a pointer?

Are C++ exceptions a no-no on the NDS?
You could eliminate a lot of error checking code.
Here's what LoadTexture would look like for example:
// LoadTexture: void ixTexturePool::LoadTexture( std::string file ){	// Attempt to load info file.	ixTextureInfo newInfo;	// Holds texture information.	LoadInfo( file, newInfo );	// Attempt to load the image file.	ixTexture newTexture;	// Holds texture data.	LoadImage( newInfo, newTexture );	// Attempt to load the palette file.	LoadPalette( newInfo, newTexture );		// Store texture against it's name.	m_Textures.insert( std::pair< std::string, ixTexture >( newInfo.Name, newTexture ) );}



The case statements in LoadImage do not have default: cases - they will fall-thru and leave texSize/texColors uninitialized.
- The trade-off between price and quality does not exist in Japan. Rather, the idea that high quality brings on cost reduction is widely accepted.-- Tajima & Matsubara
As Shanon mentioned above, you may want to use exceptions instead of return codes.

Also, too many comments IMO. Too many obvious comments.

// Attempt to load the image file.
if( LoadImage( newInfo, newTexture ) == false )


The method being called is named LoadImage() so the comment is obvious. In my opinion most of the comments in that file are no more than overhead.



The switch statements on lines 121, 137 and 214 are good candidates for extracting a new method.
private int GetTextureSizeEnum(int size){        switch( info.Size )        {        case 64:                texSize = TEXTURE_SIZE_64;                break;         case 128:                texSize = TEXTURE_SIZE_128;                break;         case 256:                texSize = TEXTURE_SIZE_256;                break;        }}private int GetColorFlag(int colors){        switch( info.Colours )        {        case 4:                texColours = GL_RGB4;                break;         case 16:                texColours = GL_RGB16;                break;         case 256:                texColours = GL_RGB256;                break;        }}


You may want to add a default case and throw an exception there.



The method LoadInfo() deals with loading an info file and fills an ixTextureInfo. You may want to consider moving this method to the ixTextureInfo class.

In that light, you can also move a lot of functionality to the ixTexture class. Things like Bind(), Load(binaryData), Unload(). Most things that are pre/suffixed with "Texture" can be put in the ixTexture class.


These comments have little to do with actual C++ and more with general coding style. I hope you don't mind.
STOP THE PLANET!! I WANT TO GET OFF!!
Just an advice:
To have more control over your copy behaviour define == and = operators (for instance if you want to avoid copies just make them private). And add a copy constructor. Otherwise the compiler will generate one and with raw pointers you can get some trouble.

With best regards

Kimmi
A complicate solution may indicate a not understood problem.


[twitter]KimKulling[/twitter]
Thanks all! That was exactly what I was looking for, I can see several things I need to read up on and learn a bit more about. Sorry for the late reply, work has been a bit busy recently.

So far I have made the following changes to my class:

* Changed to use initialiser list.

* Dropped the 'None' texture and created a BindNoTexture() func.

* Moved class into the 'ix' namespace. I was doing this to make sure I didn't conflict with anything else but the namespace makes more sense.

* Changed logic to if( condition ) and if( !condition ) etc.

* Created ReadTextFile() and ReadBinaryFile() functions to use in the Load*() functions. At the moment this return char*, going to move to vector once I understand it better.

* Created functions to set the correct flags, Get*Enum() etc.

* Reduced the commenting slightly or improved it.

About the DS tools I'm using:

It turns out I can actually use exceptions and C++ IO. The IO used to not work but does now it seem after a quick test. The compiler is GCC and the default makefiles come with no-rtti and no-exceptions flags, they are usable though, just disabled by default because of the size they add to the binary.

So onto some questions if it's not too much:

I keep thinking of std::vector as a list instead of an array. Several of you have mentioned I should use it for storing the data I read in from files. Would I just pass the first section of the vector to fread to do this for example? I'm assuming if I use C++ IO it'll be easier to use a vector for this.

Exceptions. What should and what shouldn't I trap in exceptions. I know what they are but they remind me horribly of Java from college where my tutor wrapped everything in them and the code just looked ugly. Could someone point me to a nice article about using them?

Asserts: I have no idea if these are available, but of course I could always write my own. Is there anything special I would need to do or is it just a case of writing the message to the screen if something fails?

PS:

The reason for UnloadAllTextures() is because I load/unload textures every gamestate change. I only have ~256k of texture ram in the mode I'm using which can fit about 4-5 256 colour textures. When a gamestate ends it calls UnloadAllTextures() and when one begins it loads its own ones in.

There is also no glDeleteTexture() at the moment in the lib, it would involve me writing my own VRAMmanager for it which is doable I suppose. At the moment its a case of don't screw up the files.

Thanks for all the replies so far. [smile]
I really wouldn't use exceptions on a DS. IIRC exception support in gcc adds something like a 1% or 2% overhead on all function calls (so it can rewind the stack) which is usually fine for a pc game but on a DS with limited memory you're going to waste a lot of memory you really don't have to spare. They'll screw with your cache and cause general performance overhead too.

If you really want to use them I'd suggest doing some quick profiling with and without them and see exactly what the overhead is going to be.
Quote:Original post by ZeroSum
Asserts: I have no idea if these are available, but of course I could always write my own. Is there anything special I would need to do or is it just a case of writing the message to the screen if something fails?

You don't specifically have to write to the screen. The important thing is just that 1) it notifies you *somehow* of the error (could write to a file), and 2) it halts the program.

So it's fairly easy to write a basic assert yourself.

This topic is closed to new replies.

Advertisement