Object-oriented – Is this proper OO design for C++

cdesignobject-orientedobject-oriented-design

I recently took a software processes course and this is my first time attempting OO design on my own. I am trying to follow OO design principles and C++ conventions. I attempted and gave up on MVC for this application, but I am trying to "decouple" my classes such that they can be easily unit-tested and so that I can easily change the GUI library used and/or the target OS. At this time, I have finished designing classes but have not yet started implementing methods.

The function of the software is to log all packets sent and received, and display them on the screen (like WireShark, but for one local process only). The software accomplishes this by hooking the send() and recv() functions in winsock32.dll, or some other pair of analogous functions depending on what the intended Target is. The hooks add packets to SendPacketList/RecvPacketList. The GuiLogic class starts a thread which checks for new packets. When new packets are found, it utilizes the PacketFilter class to determine the formatting for the new packet, and then sends it to MainWindow, a native win32 window (with intent to later port to Qt).1

UML Class Diagram

Full size image of UML class diagram

Here are my classes in skeleton/header form (this is my actual code):

class PacketModel
{
protected:
    std::vector<byte> data;
    int id;
public:
    PacketModel();
    PacketModel(byte* data, unsigned int size);
    PacketModel(int id, byte* data, unsigned int size);
    int GetLen();
    bool IsValid(); //len >= sizeof(opcode_t)
    opcode_t GetOpcode();
    byte* GetData(); //returns &(data[0])
    bool GetData(byte* outdata, int maxlen);
    void SetData(byte* pdata, int len);
    int GetId();
    void SetId(int id);
    bool ParseData(char* instr);
    bool StringRepr(char* outstr);
    byte& operator[] (const int index);
};

class SendPacket : public PacketModel
{
protected:
    byte* returnAddy;
public:
    byte* GetReturnAddy();
    void SetReturnAddy(byte* addy);
};

class RecvPacket : public PacketModel
{
protected:
    byte* callAddy;
public:
    byte* GetCallAddy();
    void SetCallAddy(byte* addy);
};

//problem: packets may be added to list at any time by any number of threads
//solution: critical section associated with each packet list
class Synch
{
public:
    void Enter();
    void Leave();
};

template<class PacketType> class PacketList
{
private:
    static const int MAX_STORED_PACKETS = 1000;
public:
    static const int DEFAULT_SHOWN_PACKETS = 100;
private:
    vector<PacketType> list;
    Synch synch; //wrapper for critical section
public:
    void AddPacket(PacketType* packet);
    PacketType* GetPacket(int id);
    int TotalPackets();
};

class SendPacketList : PacketList<SendPacket>
{

};

class RecvPacketList : PacketList<RecvPacket>
{

};

class Target //one socket
{
    bool Send(SendPacket* packet);
    bool Inject(RecvPacket* packet);
    bool InitSendHook(SendPacketList* sendList);
    bool InitRecvHook(RecvPacketList* recvList);
};

class FilterModel
{
private:
    opcode_t opcode;
    int colorID;
    bool bFilter;
    char name[41];
};

class FilterFile
{
private:
    FilterModel filter;
public:
    void Save();
    void Load();
    FilterModel* GetFilter(opcode_t opcode);
};

class PacketFilter
{
private:
    FilterFile filters;
public:
    bool IsFiltered(opcode_t opcode);
    bool GetName(opcode_t opcode, char* namestr); //return false if name does not exist
    COLORREF GetColor(opcode_t opcode); //return default color if no custom color
};

class GuiLogic
{
private:
    SendPacketList sendList;
    RecvPacketList recvList;
    PacketFilter packetFilter;
    void GetPacketRepr(PacketModel* packet);
    void ReadNew();
    void AddToWindow();
public:
    void Refresh(); //called from thread
    void GetPacketInfo(int id); //called from MainWindow
};

I'm looking for a general review of my OO design, use of UML, and use of C++ features. I especially just want to know if I'm doing anything considerably wrong.

From what I've read, design review is on-topic for this site (and off-topic for the Code Review site).

Any sort of feedback is greatly appreciated. Thanks for reading this.

Best Answer

Naming is important

What is a SendPacket? How is it different to a RecvPacket? Surely sending and receiving are operations performed on packets, rather than types of packet?

For that matter, what is a PacketModel? It looks like a packet, unless there's some metadata I don't see. Is it supposed to store the byte contents of one packet, or somehow describe a type of packet?

If packets are the same type regardless of whether you're sending or receiving them, you only need one type of list to contain them.

What does Target mean? It's a really generic word - is it supposed to represent a network host/peer/IP address, or an IP+port, or some internal observer, or something else? In a comment you say it's a Socket - so why not just call it a Socket?

It isn't clear why you Send a SendPacket but Inject a RecvPacket. Where do you inject it? If it is supposed to be a received packet injected by the script engine, then Target is a mixture of an abstract socket - which is a source of incoming packets and a destination for outgoing packets - and some scripted mock socket. That seems like a potentially useful distinction.

Make your abstractions flexible

Your current PacketList is your interface to the GUI, which presumably has its own thread, but you've made it impossible to use a good callback/signalling mechanism. The GUI has to either block or busy-wait calling GetPacket on each list sequentially. This is either inefficient, or guaranteed to be unresponsive, or possibly both.

It would be better to have a callback mechanism to the GUI, so an implementation can choose the best way to dispatch and synchronize events to the GUI thread on a given platform (although you need to work out how to manage the lifetime of these packet objects).

Couple carefully

Why is your FilterModel coupled to the GUI - is because you're mainly using it to colour packets? This appears to prevent you discarding packets you're not interested in before passing them up to the GUI, which seems wasteful.

Specifics

I'm not sure what all the addy stuff is supposed to accomplish - you could just expose a const ref to the underlying vector. That would be safer (unless you want to allow modification of received packet data), and cleaner. I'm also not sure what StringRepr is supposed to do - writing into an un-checked char*? Either pass a buffer size so you can check for overflow, or better implement a std::ostream& operator<< (std::ostream&, Packet const &) and let the stream worry about overflows and memory management.

Related Topic