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 GetResponse : public Response {
T value;
...
};
template
class SetRequest : 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.