Sign in to follow this  

Critique my C++.

This topic is 3410 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I'd like to get some advice on my C++ skills. I'm slowly learning but feel I'm not seeing or learning some important parts. If someone could critique my class below and give their opinion my code it'd be great. Along the lines of.. Am I doing something in a really stupid way? Causing memory leaks? Am I not using something I should be (const?), should I split the functions up further? Anything else like 'bad' C++. Below is my texture pool class for my DS project. I picked it as it's not too long and it's one of the more complete ones. It's slightly over commented at the moment. Header Source Please be as mean as you like [grin].

Share this post


Link to post
Share on other sites
Quote:
Original post by janta
Pretty good code, though why mixing C and C++? (FILE instead of file streams, char* instead of strings)
The C stdio functions are a perfectly legitimate way to perform binary I/O in C++. I would however recommend that you include the correct header; <cstdio> is the std-c++ form of <stdio.h> (you got this one right for <stdlib>).

Share this post


Link to post
Share on other sites
Thanks for the replies so far. Not as bad as I thought it is then I think. [smile]

I'm using libfat for file i/o on the DS and it provides it's own versions of fread() etc. in fat.h. There isn't a 'C++' version or another lib and this is the 'default' homebrew ds one. Ah, thanks for the reminder about the header. Have a habit of doing that when looking for what function is in what header and just copying the first I find.

Share this post


Link to post
Share on other sites
Quote:
Original post by swiftcoder
Quote:
Original post by janta
Pretty good code, though why mixing C and C++? (FILE instead of file streams, char* instead of strings)
The C stdio functions are a perfectly legitimate way to perform binary I/O in C++.


C style i/o is incompatible with the rest of the C++ standard library like generic algorithms for example. C++ i/o also provides safety by internalizing buffer management and adding type safety. For those and other reasons I prefer to use C++ i/o

To the OP, if you have a switch-statement or an if-statement where there is an "impossible case", you should explicitly handle it by inserting an "assert(false)". That way you are instantly alerted if your program enters one of those "impossible states".


switch(someValue){
case legitCaseA:
...
break;
case legitCaseB:
...
break;
default:
/* should be impossible */
assert(false); /* make sure to #include <cassert>*/
}



Share this post


Link to post
Share on other sites
You should consider std::vector<char> in lieu of char* imageData = new char[]. vector is meant as a replacement for dynamically allocated array. It uses RAII model so you don't have to worry about memory leaks. It has the subscript operator to provide use in conjunction with C APIs. The memory in vector is guaranteed to be contiguous.

Share this post


Link to post
Share on other sites
A few specific notes:

source line 11: I'm not sure why you are using the append funtion. You can just use the assignment function, m_BaseDir = baseDir; This is clearer and also faster. Even better would be to initialize m_BaseDir in the constructor initializer list which would be fastest since it would use the string copy constructor.

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.

source line 45: If LoadPalette fails, then you have an opengl resource leak. The previous call to LoadImage allocated a texture, it must be freed (with glDeleteTextures) before returning false from ixTexturePool::LoadTexture

source line 57: You use the magic texture string ID "None" in order to unbind. For my tastes this is bad. I recommend a seperate function UnbindTexture() for this purpose. I think it would make usage of the class clearer, plus it would have the added bonus of being faster :)

source line 69: Silently ignoring errors. This is a tricky one. In general, I think it's bad to silently ignore errors like this. It can lead to nightmarish debug sessions later down the line. In this specific case I am not sure what to do about it. Returning an error code is bad since no one will check it anyway. If you decide that it's a programming error to try to bind a non-existent error, then you can use "assert" as suggested above by fpsgamer. Otherwise, I guess you could keep the code the way it is, but maybe consider at least logging the error to stderr or something.

source line 242: ixTexturePool::LoadInfo function. Above fpsgamer mentioned adding "assert" calls to your code to catch things like invalid info.Size. Sprinkling code with asserts is always good, but in your code this is not enough. You are reading this data from a file, and the proper thing to do is validate each datum of the file. So just after line 304, you should check that info.Size is a valid value, and if not, return false from the function(after possibly writing to some error log the specific reason of failure). You should do checks like this for every piece of the file. Yeah, it's a pain in the ass but it's really the right thing to do, even though that these texture files are part of the game and you know that they contain only valid data. For example, in the future if you change your file format and update all your data files to comply, but somehow an old version file sneaks through, you get a helpful error message about invalid data instead of your program just crashing, potentially long after the file has been loaded, leaving you with no clue where the actual problem is.

A few general notes:

Your comments could be better

// Unloads all textures.
void UnloadAllTextures();

That doesn't really help anyone :) Good commenting is one of the hardest parts of programming, but it's also one of the most important, you might want to try searching for one of the many online articles with advice about how to comment properly.

The code for the debug functions are so short and in the typical "get" style, that it could be moved directly into the class definition in the header file. This will make them inlined and faster. Beware though that this will also have an effect on binary compatibility if you later change their implementation. For these debug functions the performance is probably not an issue, so it doesn't really matter. I just wanted to point this out in case you weren't aware of this convention.

You don't seem to use "const" anywhere. Placing const qualifiers in various places is a simple feature of C++ that makes your code more robust and faster. All 3 debug functions can be declared "const". Also, you're functions that accept string arguments can instead accept const string references, which is just as safe as passing strings by value but a lot faster.

bool LoadTexture( std::string file);
bool LoadTexture( const std::string& file);

About the issue of C vs C++ i/o: It basicly comes down to exception usage. You don't seem to be using exceptions, so sticking with the C functions is fine and probably better. However, you are using C++ code that can cause exceptions("new" keyword, the STL library). My advise for you is to just "be careful" with those functions.

I've mentioned "performance" here a lot. In general, a lot of factors are much more important than performance, such as code readability and robustness. Most of my suggestions are directly targeted at improving code readability and robustness, and improved performance is just a happy side effect. C++ gets a lot of hate, but in my opinion it is well designed and in a lot of cases the simplest most elegant code is also the most performant.

Other than all this, the code is overall pretty good! :) There are other programming styles that some people think are more "professional" or what some people call "Modern C++". I am just mentioning this just so that I can be honest and say that there are even more suggestions that I could give you. But there are lots of ways to program, especially in a language as powerful as C++, and a lot of times keeping things simple like the way you have done is the best, and is often done in game programming.

Share this post


Link to post
Share on other sites
Don't prefix everything with 'ix'. Make an 'ix' namespace instead.

Pass std::string arguments (in general, arguments of object type) by const reference, by default.

Instead of .append() for strings, I would argue operator+= is more idiomatic.

Use initializer lists in constructors. Actually, it's quite strange to use append() to "initialize" your BaseDir member (which relies on the fact that it's been already default-initialized).

Don't compare to boolean literals. Just use boolean conditions directly (or invert them with '!' if need be). How often do you catch yourself speaking the phrase "if it is true that"?

If you are going to insert unconditionally, the operator[] of maps is more idiomatic. But even if you're concerned about efficiency (since operator[] will default-construct and then assign over top for a new element), use the std::make_pair helper function to avoid having to specify template arguments for std::pair explicitly.

Using "None" as a "magic" string for unregistration is dubious. Why not use an empty string? That's much less likely to be a valid texture name :)

Consider not using guard clauses quite so much, especially if they're not at the top of the function.

Use references to avoid repeating common subexpressions. For example, when iterating through a map, or working with a found element it might be a good idea to make a reference to 'it->second'. Similarly, you ought to be able to do something about "textInfo.substr( lineSep + 2, lineEnd - (lineSep + 2))".

Is UnloadAllTextures() intended to be called explicitly by the user? Why would this be desirable? Why not just let the pool fall out of scope and reconstruct it?

If you're smart enough to use std::string, you should be smart enough to use std::vector, as noted.

If you have access to exceptions, use them. This "return a bool success code and represent the actual result as a change to an input parameter" business is pervasive in the industry, but ultimately it just complicates things. (I've seen code with huge amounts of logic to propagate errors manually where (a) an exception, allowed to propagate back to the initial call, would do the same, and (b) I actually managed to prove statically that the error was impossible, given that there hadn't already been a should-be-fatal error previously.)

Factor out similarities to emphasize differences, instead of emphasizing the need to handle the differences. Example:


texFlags = TEXGEN_TEXCOORD | GL_TEXTURE_WRAP_S | GL_TEXTURE_WRAP_T;
if (info.Transparent) {
texFlags |= GL_TEXTURE_COLOR0_TRANSPARENT;
}


Make a helper function for the "load a whole file into a buffer" process. Avoid repetition.

I can't believe you have access to std::string but not the standard I/O streams. Seriously, what happens if you try to #include <iostream>?

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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]

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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

Share this post


Link to post
Share on other sites
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]

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites

This topic is 3410 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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

Sign in to follow this