C++ Programming Puzzle
Monday July 7, 2008
Here's some source code for a class to manage planets in an astronomy program. A planet is created with co-ordinates (assume those are correct- the values are not the problem) and a name. So I've added Earth. This code works BTW, it prints out Earth. But it's flawed. How many bugs can you spot?
#include <iostream>
#include <string>
using namespace std;
class Planet
{
private:
float x,y,z;
char *PlanetName;
public:
Planet(float X, float Y, float Z, char * NewName ) ;
void SetPlanetName( char * NewName ) ;
char * GetPlanetName() {return PlanetName;};
};
Planet::Planet(float X,float Y, float Z, char * NewName )
: x(X),y(Y),z(Z)
{
SetPlanetName( NewName ) ;
}
void Planet::SetPlanetName( char * NewName )
{
PlanetName = new char[strlen(NewName) + 1];
strcpy(PlanetName, NewName) ;
}
int main(int argc, char* argv[])
{
Planet Earth(10789.0f,102345.4f,10234.23f,"Earth") ;
cout << Earth.GetPlanetName() << endl;
return 0;
}
Answer on Wednesday.
- Link to C++ Tutorials
There’s so many. Firstly, this uses raw pointers rather than a smart pointer or even std::string.
The Planet constructor isn’t exception safe – if setPlanetName throws then the class is left with side effects
You never check newName for NULL
GetPlanetName returns a pointer to private data breaking encapsulation
Meyers recommends that you use custom types (maybe just structs or typedefs) for XYZ so that it is more difficult to use the interface incorrectly (i.e. something like:
#define float X-CoOrd;
#define float Y-CoOrd;
#define float Z-CoOrd;
and then use these in the signature for the fcn so the compiler will alert you if you try to supply them in the wrong order)
The most egregious bug I see is that PlanetName leaks when a Planet object is destroyed.
Use of raw pointers isn’t strictly speaking a bug as much as a poor design decision (oddly, string is #included at the top of the source code). You can carry this over to the bug I mentioned above, plus the fact that SetPlanetName leaks memory if called again outside the constructor. The leak ought to be fixed, or SetPlanetName ought to be declared private. The char* approach could work if the memory were destroyed appropriately.
Planet’s constructor isn’t exception safe, but it’s not too bad. The worst possible side-effect is a memory leak if strcpy throws. In general, strcpy should either work correctly or generate undefined behavior, not throw.
Failure to check for NULL and returning a pointer to private data is not necessarily a bug; it depends on the context of the class. If Planet is a simple utility class not to be exposed to end-users then such standards can generally be relaxed.
I think you’ll find that those #defines are buggier than anything in the code right now. If the coordinates can include the full gamut of float values then there’s no reason for custom types. XYZ coordinates might need to intermingle (such as in calculating a cross product), so putting up barriers to usability isn’t necessarily a good idea.
I agree with the above comments entirely. I’d add that you could specify the usage of a few consts here and there. Get functions can return and be specified as consts, in parameters could be const (at least for the char* or string). Probably don’t care since the floats are passed by value.
I agree with Mike’s comments as well. The only bug that I can find is the memory leak in SetPlanetName.
I would have written the class like this:
class Planet
{
public:
Planet(const std::string & name, float x, float y, float z) : mName(name), mX(x), mY(y), mZ(z)
{
}
const std::string & name() const
{
return mName;
}
private:
std::string mName;
float mX, mY, mZ;
};