Critique my C++.
Author
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].
I only gave it a quick glance, but anyway: no glaring problems, well commented, consistent syntax, etc. Good job.
Pretty good code, though why mixing C and C++?
(FILE instead of file streams, char* instead of strings)
(FILE instead of file streams, char* instead of strings)
Quote:Original post by jantaThe 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>).
Pretty good code, though why mixing C and C++? (FILE instead of file streams, char* instead of strings)
Author
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.
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.
Quote:Original post by swiftcoderQuote:Original post by jantaThe C stdio functions are a perfectly legitimate way to perform binary I/O in C++.
Pretty good code, though why mixing C and C++? (FILE instead of file streams, char* instead of strings)
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>*/}
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.
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.
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.
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:
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>?
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>?
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
