Code Complete 2ed, composition and delegation

Question

After a couple of weeks reading on this forum I thought it was time for me to do my first post.

I'm currently rereading Code Complete. I think it's 15 years since the last time, and I find that I still can't write code ;-)

Anyway on page 138 in Code Complete you find this coding horror example. (I have removed some of the code)

class Emplyee {
public: 
 FullName GetName() const;
 Address GetAddress() const;
 PhoneNumber GetWorkPhone() const;
 ...

 bool IsZipCodeValid( Address address);
 ...

private: 
   ...
}

What Steve thinks is bad is that the functions are loosely related. Or has he writes "There's no logical connection between employees and routines that check ZIP codes, phone numbers or job classifications"

Ok I totally agree with him. Maybe something like the below example is better.

class ZipCode
{
public:
 bool IsValid() const;
    ...
}

class Address {
public:
   ZipCode GetZipCode() const;
   ...
}

class Employee {
public: 
 Address GetAddress() const;
    ...
}

When checking if the zip is valid you would need to do something like this.

employee.GetAddress().GetZipCode().IsValid();

And that is not good regarding to the Law of Demeter.

So if you like to remove two of the three dots, you need to use delegation and a couple of wrapper functions like this.

class ZipCode
{
public:
 bool IsValid();
}

class Address {
public:
   ZipCode GetZipCode() const;
   bool IsZipCodeValid() {return GetZipCode()->IsValid());
}

class Employee {
public: 
 FullName GetName() const;
 Address GetAddress() const;
 bool IsZipCodeValid() {return GetAddress()->IsZipCodeValid());
 PhoneNumber GetWorkPhone() const;
}

employee.IsZipCodeValid();

But then again you have routines that has no logical connection.

I personally think that all three examples in this post are bad. Is it some other way that I haven't thought about?

Answer 1

It's pay now vs. pay later.

You can write the delegation and wrapper functions up front (pay now) and then have less work changing the innards of employee.IsZipCodeValid() later. Or, you can tunnel through to IsZipCodeValid by writing

employee.GetAddress().GetZipCode().IsValid();

everywhere you need it in the code, but pay later should you decide to change your class design in a way which breaks this code.

You get to choose your poison. ;)

Answer 2

You are missing the logical connection:

class ZipCode
{
public:
 bool IsValid();
}

class Address {
public:
   ZipCode GetZipCode() const;
   bool IsAddressValid();
   bool IsValid() {return GetZipCode()->IsValid() && IsAddressValid());
}

class Employee {
public: 
 FullName GetName() const;
 Address GetAddress() const;
 bool IsEmployeeValid();
 bool IsValid() {return GetAddress()->IseValid() && IsEmployeeValid());
 PhoneNumber GetWorkPhone() const;
}

employee.IsValid();

Answer 3

Since there's no logical connection between the Employee class and zip-code validation, you could put the Zip code validation into the Address class where it more logically belongs. Then you can ask the Address class to validate the Zip code for you.

class Address
{
    public:
        static IsZipValid(ZipCode zip) { return zip.isValid(); }
};

Then you do

Address::IsZipValid(employee.GetAddress().GetZipCode());

I think this is satisfactory under your constraints of logical association and Law of Demeter.