Style Case Studies: Construction Unions - Into the Code
(Page 4 of 9 )
Let’s now consider the code:
#include <list>
#include <string>
#include <iostream>
using namespace std;
Always include necessary headers. Because new is going to be used below, we need to also #include <new>. (Note: The <iostream> header is okay; later in the original code, not shown here, was a test harness that emitted output using iostreams.)
#define max(a,b) (a)>(b)?(a):(b)
typedef list<int> LIST;
typedef string STRING;
struct MYUNION {
MYUNION() : currtype( NONE ) {}
~MYUNION() {cleanup();}
The first classic mechanical error here is that MYUNION is unsafe to copy because the programmer forgot to provide a suitable copy constructor and copy assignment operator.
MYUNION is choosing to play games that require special work to be done in the constructor and destructor, so these functions are provided explicitly as shown; that’s fine as far as it goes. But it doesn’t go far enough, because the same games require special work in the copy constructor and copy assignment operator, which are not provided explicitly. That’s bad because the default compiler-generated copying operations do the wrong thing; namely, they copy the contents bitwise as a char array, which is likely to have most unsatisfactory results, in most cases leading straight to memory corruption. Consider the following code:
// Example 36-3: MYUNION is unsafe for copying
//
{
MYUNION u1, u2;
u1.getstring() = “Hello, world”;
u2 = u1; // copies the bits of u1 to u2
} // oops, double delete of the string (assuming the bitwise copy even made sense)
Guideline: Observe the Law of the Big Three [Cline99]: If a class needs a custom copy constructor, copy assignment operator, or destructor, it probably needs all three.
Passing on from the classic mechanical error, we next encounter a duo of classic stylistic errors:
enum uniontype {NONE,_INT,_LIST,_STRING};
uniontype currtype;
inline int& getint();
inline LIST& getlist();
inline STRING& getstring();
There are two stylistic errors here. First, this struct is not reusable because it is hardcoded for specific types. Indeed, the original article recommended hand-coding such a struct every time it was needed. Second, even given its limited intended usefulness, it is not very extensible or maintainable. We’ll return to this frailty again later, once we’ve covered more of the context.
Guideline: Avoid hard-wiring information that needlessly makes code more brittle and limits flexibility.
There are also two mechanical problems. The first is that currtype is public for no good reason; this violates good encapsulation and means any user can freely mess with the type flag, even by accident. The second mechanical problem concerns the names used in the enumeration; I’ll cover that in its own section, “Underhanded Names,” later on.
protected:
Here we encounter another mechanical error: The internals ought to be private, not protected. The only reason to make them protected would be to make the internals available to derived classes, but there had better not be any derived classes because MYUNION is unsafe to derive from for several reasons—not least because of the murky and abstruse games it plays with its internals and because it lacks a virtual destructor.
Guideline: Always make all data members private. The only exception is the case of a C-style struct which isn’t intended to encapsulate anything and where all members are public.
 This chapter is from Exceptional C++ Style, by Herb Sutter (ISBN 0201760428, copyright 2005. All rights reserved. It is reprinted with permission from Addison-Wesley Professional). Check it out at your favorite bookstore today.
Buy this book now. |
Next: Comments >>
More Code Examples Articles
More By Addison-Wesley/Prentice Hall PTR
/ 8 
