Comments are used to document your source code so that when a person reads it he knows what he's looking at. Comments should be used in a manner that allows the user to know, beyond the snippet of information passed by the name of a function or variable, what that function or variable does, how it is used, and whatever else may be useful to the programmer and the reader.
//----------------------------------------------------------------------------//
// Class declaration //
//----------------------------------------------------------------------------//
class Model
{
// misc
public:
static const int STATE_IDLE;
static const int STATE_FANCY;
static const int STATE_MOTION;
// member variables
protected:
int m_state;
CalCoreModel* m_calCoreModel;
CalModel* m_calModel;
int m_animationId[16];
int m_animationCount;
int m_meshId[32];
int m_meshCount;
GLuint m_textureId[32];
int m_textureCount;
float m_motionBlend[3];
float m_renderScale;
float m_lodLevel;
std::string m_path;
// constructors/destructor
public:
Model();
virtual ~Model();
// member functions
public:
void executeAction(int action);
float getLodLevel();
void getMotionBlend(float *pMotionBlend);
float getRenderScale();
int getState();
bool onInit(const std::string& strFilename);
void onRender();
void onShutdown();
void onUpdate(float elapsedSeconds);
void setLodLevel(float lodLevel);
void setMotionBlend(float *pMotionBlend, float delay);
void setState(int state, float delay);
void setPath( const std::string& strPath );
/* DEBUG-CODE
struct
{
float x, y, z;
} Sphere;
void adjustSphere(float x, float y, float z) { Sphere.x += x; Sphere.y += y; Sphere.z += z; };
*/
protected:
GLuint loadTexture(const std::string& strFilename);
void renderMesh(bool bWireframe, bool bLight);
void renderSkeleton();
void renderBoundingBox();
};
|
This is a perfect example of improper comments. A well educated C++ coder would never expect that those marked as member variables are actually member variables, nor would he know that the functions Model() and ~Model() are constructor and destructor, respectively. Also, note the "member functions" comment for the member functions. All of the static const variables are marked as "misc." I have yet to find out what's so "misc" about them. Nothing in my coding is "misc" and I doubt anything in this code is either. A brief survey of all of the structures in the Cally Demo will show the same comments used there as well, one might think the author has a class template with comments ready to roll.
I wish to call your attention to the Model() and ~Model() functions, in the model.cpp file. The following code is the entirety of the constructor and destructor, unedited. Note the comment says "Constructors," plural, not only here, but in the header file as well. There's only one constructor, and as you will shortly find out, it's not really a whole constructor.
//----------------------------------------------------------------------------//
// Constructors //
//----------------------------------------------------------------------------//
Model::Model()
{
m_calCoreModel = new CalCoreModel("dummy");
m_state = STATE_IDLE;
m_motionBlend[0] = 0.6f;
m_motionBlend[1] = 0.1f;
m_motionBlend[2] = 0.3f;
m_animationCount = 0;
m_meshCount = 0;
m_renderScale = 1.0f;
m_lodLevel = 1.0f;
/* DEBUG-CODE
Sphere.x = 0.0f;
Sphere.y = 5.0f;
Sphere.z = 45.0f;
*/
}
//----------------------------------------------------------------------------//
// Destructor //
//----------------------------------------------------------------------------//
Model::~Model()
{
}
|
The destructor is plugged, this is the first warning sign. The constructor seems to be doing some initialization, however, I alert you to the fact that it does not actually construct anything, it only sets all the variables to some form of default value. The real constructing is done by the following function:
//----------------------------------------------------------------------------//
// Initialize the model //
//----------------------------------------------------------------------------//
bool Model::onInit(const std::string& strFilename)
{
// open the model configuration file
std::ifstream file;
file.open(strFilename.c_str(), std::ios::in | std::ios::binary);
if(!file)
{
std::cerr << "Failed to open model configuration file '" << strFilename << "'." << std::endl;
return false;
}
// initialize the data path
std::string strPath = m_path;
// initialize the animation count
int animationCount;
animationCount = 0;
// parse all lines from the model configuration file
int line;
for(line = 1; ; line++)
{
// read the next model configuration line
std::string strBuffer;
std::getline(file, strBuffer);
// stop if we reached the end of file
if(file.eof()) break;
// check if an error happend while reading from the file
if(!file)
{
std::cerr << "Error while reading from the model configuration file '" << strFilename << "'." << std::endl;
return false;
}
// find the first non-whitespace character
std::string::size_type pos;
pos = strBuffer.find_first_not_of(" \t");
// check for empty lines
if((pos == std::string::npos) || (strBuffer[pos] == '\n') || (strBuffer[pos] == '\r') || (strBuffer[pos] == 0)) continue;
// check for comment lines
if(strBuffer[pos] == '#') continue;
// get the key
std::string strKey;
strKey = strBuffer.substr(pos, strBuffer.find_first_of(" =\t\n\r", pos) - pos);
pos += strKey.size();
// get the '=' character
pos = strBuffer.find_first_not_of(" \t", pos);
if((pos == std::string::npos) || (strBuffer[pos] != '='))
{
std::cerr << strFilename << "(" << line << "): Invalid syntax." << std::endl;
return false;
}
// find the first non-whitespace character after the '=' character
pos = strBuffer.find_first_not_of(" \t", pos + 1);
// get the data
std::string strData;
strData = strBuffer.substr(pos, strBuffer.find_first_of("\n\r", pos) - pos);
// handle the model creation
if(strKey == "scale")
{
// set rendering scale factor
m_renderScale = atof(strData.c_str());
}
else if(strKey == "path")
{
// set the new path for the data files if one hasn't been set already
if (m_path == "") strPath = strData;
}
else if(strKey == "skeleton")
{
// load core skeleton
std::cout << "Loading skeleton '" << strData << "'..." << std::endl;
if(!m_calCoreModel->loadCoreSkeleton(strPath + strData))
{
CalError::printLastError();
return false;
}
}
else if(strKey == "animation")
{
// load core animation
std::cout << "Loading animation '" << strData << "'..." << std::endl;
m_animationId[animationCount] = m_calCoreModel->loadCoreAnimation(strPath + strData);
if(m_animationId[animationCount] == -1)
{
CalError::printLastError();
return false;
}
animationCount++;
}
else if(strKey == "mesh")
{
// load core mesh
std::cout << "Loading mesh '" << strData << "'..." << std::endl;
if(m_calCoreModel->loadCoreMesh(strPath + strData) == -1)
{
CalError::printLastError();
return false;
}
}
else if(strKey == "material")
{
// load core material
std::cout << "Loading material '" << strData << "'..." << std::endl;
if(m_calCoreModel->loadCoreMaterial(strPath + strData) == -1)
{
CalError::printLastError();
return false;
}
}
else
{
std::cerr << strFilename << "(" << line << "): Invalid syntax." << std::endl;
return false;
}
}
// explicitely close the file
file.close();
// load all textures and store the opengl texture id in the corresponding map in the material
int materialId;
for(materialId = 0; materialId < m_calCoreModel->getCoreMaterialCount(); materialId++)
{
// get the core material
CalCoreMaterial *pCoreMaterial;
pCoreMaterial = m_calCoreModel->getCoreMaterial(materialId);
// loop through all maps of the core material
int mapId;
for(mapId = 0; mapId < pCoreMaterial->getMapCount(); mapId++)
{
// get the filename of the texture
std::string strFilename;
strFilename = pCoreMaterial->getMapFilename(mapId);
// load the texture from the file
GLuint textureId;
textureId = loadTexture(strPath + strFilename);
// store the opengl texture id in the user data of the map
pCoreMaterial->setMapUserData(mapId, (Cal::UserData)textureId);
}
}
// make one material thread for each material
// NOTE: this is not the right way to do it, but this viewer can't do the right
// mapping without further information on the model etc.
for(materialId = 0; materialId < m_calCoreModel->getCoreMaterialCount(); materialId++)
{
// create the a material thread
m_calCoreModel->createCoreMaterialThread(materialId);
// initialize the material thread
m_calCoreModel->setCoreMaterialId(materialId, 0, materialId);
}
// Calculate Bounding Boxes
m_calCoreModel->getCoreSkeleton()->calculateBoundingBoxes(m_calCoreModel);
m_calModel = new CalModel(m_calCoreModel);
// attach all meshes to the model
int meshId;
for(meshId = 0; meshId < m_calCoreModel->getCoreMeshCount(); meshId++)
{
m_calModel->attachMesh(meshId);
}
// set the material set of the whole model
m_calModel->setMaterialSet(0);
// set initial animation state
m_state = STATE_MOTION;
m_calModel->getMixer()->blendCycle(m_animationId[STATE_MOTION], m_motionBlend[0], 0.0f);
m_calModel->getMixer()->blendCycle(m_animationId[STATE_MOTION + 1], m_motionBlend[1], 0.0f);
m_calModel->getMixer()->blendCycle(m_animationId[STATE_MOTION + 2], m_motionBlend[2], 0.0f);
return true;
}
|
Whoa, perhaps that's a tad long. Whatever. As you can see, there's a lot going on in the construction of this class... But why is it in an init function? I don't know. The reason why C++ has constructors is because C++ wishes to offer the programmer a fully constructed and ready to use object coming straight out of new. Adding an init function seperate from the constructor completely defeats this, and makes for an opportunity for the user of the class (or object) to try using the object before it is properly constructed. Bad juju. There is only one instance where I had to use an init function for a class, and that was when I needed to initialize certain things in a different thread of execution. A programmer must have a very good excuse to have or use an init function, but this programmer has no excuse.
The quality of the comments, no matter how terse, is a lot better inside this function, but the comments where they are truly needed by any person using this class are very lacking. This is probably because the code here is not intended to be reused, but read and analyzed for its use of the Cal3d library.
Similar to the constructor, the destructor was plugged and reimplimented in the following function:
//----------------------------------------------------------------------------//
// Shut the model down //
//----------------------------------------------------------------------------//
void Model::onShutdown()
{
delete m_calModel;
delete m_calCoreModel;
}
|
As stated before, these comments describing what the function does are horribly useful. I couldn't possibly imagine a function named Model::onShutdown() that actually shuts down the model. Having a plugged destructor and a shutdown function that takes its place is bad because the programmer can delete this object without ever cleaning it up. In this case, the onShutdown only calls delete on a few blocks of memory held by the object, causing a memory leak if it is not called. If this object were created and destroyed a few hundred times per minute, the program would quickly run out of memory. This is not so bad, considering it probably will be called when the application shuts down anyway, but imagine this object needed to take itself out of a list of objects, like an event acceptor list... The results could be disasterous and hard to debug.
When describing a function, be careful not to state things that it doesn't do. In this case, the programmer "avoids" the big endian vs little endian issue with the following function:
//----------------------------------------------------------------------------//
// Read a int from file stream (to avoid Little/Big endian issue)
//----------------------------------------------------------------------------//
int readInt( std::ifstream *file )
{
int x = 0;
for ( int i = 0; i < 32; i+=8 )
{
char c;
file->read ( &c, 1 );
x += c << i;
}
return x;
}
|
Note that this is the first descriptive function description in the source code file. I think it's the only one, and the description clearly states that this function defrays the "Little/Big endian issue." This is, of course, if you're on a little endian machine. A big endian machine will misread the integer. Perhaps if the author should have said "Reads integers from the file stream on little endian machines." Be very careful when describing things in your comments, it may cost you or somebody else hours of debugging in the future.
All of these examples are bad practices that you will find in semi-professional and professional code. These are not broken programs, by far, but the problems presented will cause somebody, somehwere, a considerable amount of grief. This is bad coding style put to practice. I really wish there were compilers that could catch this type of bad programming. The programmers should learn why the language they are using is written in the way it is written, and not circumvent the constructs that have been put in place to keep them from screwing up. Some may theorize that in the future there may be a programming language that will reduce all logic errors to compile time errors. I think not. Programmers will always find ways to circumvent the constructs of the languages put in place to protect them from their own costly mistakes.