Refactoring Monster Classes

In a "spare time" project, I've been experimenting with all the "bright ideas" I'm not allowed to use at work: TestDrivenDevelopment, DoTheSimplestThingThatCouldPossiblyWork, ReFactoring, etc. Recently I came to a point where I remembered some old code I'd written a few years ago when I first went from C to C++, and thought it might be a good fit.

Ugh.

It was a monster-sized class, and the LargeClass stench rose from it in visible clouds. It implements a text-based, request-response network protocol almost but not quite entirely unlike XML. The text-based messages are of the variety:

GetObjectColor(door)
GetObjectColorResponse(door, ok, white)
GetObjectStateResponse(door, failed, no_door_found)

SetObjectColor(door, white)
SetObjectColor(door, white, ok)
SetObjectColor(door, white, failed, out_of_white_paint)

SendObjectCommand(door, close)
SendObjectCommandResponse(door, close, ok)
SendObjectCommandResponse(door, close, failed, foot_caught_in_door)

Obviously, the actual messages were a lot more complicated, but these will do. There are fifteen of these action/response pairs; every action message has a corresponding response. Unfortunately, coming straight from (very poor) C coding practices, I made One Big Class to handling parsing of these things. It went something like this:

class Message {
   enum {GET_COLOR, GET_COLOR_RESPONSE,
         SET_COLOR, SET_COLOR_RESPONSE,
         SEND_COMMAND, SEND_COMMAND_RESPONSE, ...
   }, commandType;
  string rawText;
  ParseMessage(const string& text);
  SetObjectTarget(const string& obj);
  SetState(const string& obj); // ONLY USE ON SET AND COMMAND! 
  SetResult(const string& result, const string& reason); // ONLY USE ON RESPONSES!
},

Yuck. Lots of refactorings can be done: switch statements (in ParseMessage) and large class for starters.

So I opened the RefactoringBook and sure enough, it listed four refactorings: for "large class": Extract Class, Extract Subclass, Extract Interface, and Replace Data Value with Object. I chose Extract Subclass, so then I had this:

class Message {},;
  class GetColorMessage : public Message {},;
  class GetColorResponse : public Message {},;
  class SetColorMessage : public Message {},;
  class SetColorResponse : public Message {},;

Of course, now I saw another refactoring opportunity: splitting out the "message-ness" or "response-ness". So I had this:

class MessageBase {},;
  class Request : public RequestBase {},;
    class GetColorRequest : public Request {},; 
    class SetColorRequest : public Request {},;
  class Response : public Response {},;
    class GetColorResponse : public Response {},;
    class SetColorResponse : public Response {},;

But then I came across another problem: GetColorRequest and GetColorResponse shared a lot of code. So I tried to apply ExtractSuperclass, and fell immediately into the "diamond" multiple inheritance problem:

MessageBase
    /            /  \                      
  /    \       
     Request    GetColorMessage
  \    /
   \  /
    \/
     GetColorMessageRequest

This became as bad as the original to maintain!

''So, where did I go wrong?"

You're relying on inheritance too much instead of composition. You have two types of message: request and response. Of those, some requests get a named, typed data item and others set a named, typed data item. I would structure your messages like this:

class Message { ... },;
class Request : public Message { ... },;
class Response : public Message { ... },

class GetRequest : public Request {
    std::string name;
    ...
},;

template <class T>
class GetResponse<T> : public Response {
    T value;
    ...
},;

template <class T>
class SetRequest<T> : public Request {
    std::string name;
    T new_value;
},;

class ErrorResponse : public Response {
    std::string message;
},;

And so on...

Continue refactoring, using StrategyPattern, StatePattern, BridgePattern, FactoryPattern, and more. Read AsdPpp for pointers on when and how to apply these patterns.