Questo design OO è appropriato per C ++?

2

Recentemente ho seguito un corso sui processi software e questa è la prima volta che cerco di progettare OO da solo. Sto cercando di seguire i principi di progettazione OO e le convenzioni C ++. Ho tentato e rinunciato a MVC per questa applicazione, ma sto cercando di "disaccoppiare" le mie classi in modo tale che possano essere facilmente testate unitamente e così posso facilmente cambiare la libreria GUI utilizzata e / o il SO di destinazione. Al momento, ho finito di progettare le classi ma non ho ancora iniziato a implementare i metodi.

La funzione del software è di registrare tutti i pacchetti inviati e ricevuti e visualizzarli sullo schermo (come WireShark, ma solo per un processo locale). Il software realizza ciò agganciando le funzioni send () e recv () in winsock32.dll, o qualche altra coppia di funzioni analoghe a seconda di quale sia l'obiettivo desiderato. Gli hook aggiungono pacchetti a SendPacketList / RecvPacketList. La classe GuiLogic avvia una discussione che verifica la presenza di nuovi pacchetti. Quando vengono trovati nuovi pacchetti, utilizza la classe PacketFilter per determinare la formattazione per il nuovo pacchetto e quindi lo invia a MainWindow, una finestra win32 nativa (con l'intenzione di portarla successivamente a Qt) .1

Immagine a dimensione intera del diagramma di classe UML

Ecco le mie classi in forma scheletro / intestazione (questo è il mio vero codice):

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
};

Sto cercando una revisione generale della mia progettazione OO, l'uso di UML e l'uso delle funzionalità C ++. Voglio soprattutto sapere se sto facendo qualcosa di molto sbagliato.

Da quello che ho letto, la revisione del design è in-topic per questo sito (e fuori tema per il sito Code Review).

Qualsiasi tipo di feedback è molto apprezzato. Grazie per aver letto questo.

    
posta user121917 02.03.2014 - 23:14
fonte

2 risposte

3

La denominazione è importante

Che è a SendPacket ? In che modo è diverso da un RecvPacket ? Sicuramente l'invio e la ricezione sono operazioni eseguite su pacchetti, piuttosto che tipi di pacchetti?

Se è così, che cos'è un PacketModel ? Sembra un pacchetto, a meno che non ci siano metadati che non vedo. Si suppone di memorizzare il contenuto di byte di un pacchetto, o in qualche modo descrivere un tipo di pacchetto?

Se i pacchetti sono dello stesso tipo indipendentemente dal fatto che li stia inviando o ricevendo, è sufficiente un solo tipo di elenco per contenerli.

Che cosa significa Target ? È una parola veramente generica - dovrebbe rappresentare un host di rete / peer / indirizzo IP, o una porta IP +, o qualche osservatore interno o qualcos'altro? In un commento dici che è un Socket, quindi perché non chiamarlo Socket?

Non è chiaro il motivo per cui Send a SendPacket ma Inject a RecvPacket . Dove lo si inietta? Se si suppone che sia un pacchetto ricevuto iniettato dal motore di script, allora Target è una combinazione di un socket astratto - che è una fonte di pacchetti in entrata e una destinazione per i pacchetti in uscita - e un qualche socket fittizio con script. Sembra una distinzione potenzialmente utile.

Rendi flessibili le tue astrazioni

Il tuo attuale PacketList è la tua interfaccia con la GUI, che presumibilmente ha il suo thread, ma hai reso impossibile usare un buon meccanismo di callback / signaling. La GUI deve bloccare o occupato-attendere chiamando GetPacket su ogni lista in sequenza. Questo è o inefficiente, o garantito per non rispondere, o forse entrambi.

Sarebbe meglio avere un meccanismo di callback per la GUI, quindi un'implementazione può scegliere il modo migliore per inviare e sincronizzare eventi al thread della GUI su una determinata piattaforma (anche se è necessario capire come gestire la durata di questi oggetti pacchetto).

Coppia con attenzione

Perché il tuo FilterModel è accoppiato alla GUI - è perché lo stai principalmente usando per colorare i pacchetti? Questo sembra impedirti di scartare i pacchetti che non ti interessano prima di passarli alla GUI, il che sembra uno spreco.

Specifiche

Non sono sicuro di ciò che tutte le cose addy dovrebbero realizzare - potresti semplicemente esporre un riferimento const al vettore sottostante. Ciò sarebbe più sicuro (a meno che non si desideri consentire la modifica dei dati di pacchetto ricevuti) e più pulito. Inoltre, non sono sicuro di cosa debba fare StringRepr - scrivendo in una percentuale non controllata char* ? Passa una dimensione del buffer in modo da poter controllare l'overflow o implementare meglio un std::ostream& operator<< (std::ostream&, Packet const &) e lasciare che lo stream si preoccupi degli overflow e della gestione della memoria.

    
risposta data 11.08.2014 - 19:31
fonte
1
  1. bool GetData(byte* outdata, int maxlen); perché non restituire un byte* come risultato? In generale è male avere un'uscita nel parametro. Puoi anche considerare di creare una struttura deturpata per il risultato del metodo.
  2. Non dovrebbero esserci campi protetti - è meglio avere getter protetto e setter e campo di backing privato (encaupsulation).
  3. class PacketList - Sono sicuro che ci sia già un qualche tipo di ConcurrentList implementato.
  4. Che cosa sono SendPacketList e RecvPacketList clases per? Sono inutili.
  5. Vedo che molte cose restituiscono un valore bool . Prenderò in considerazione l'utilizzo della gestione delle eccezioni.
risposta data 14.03.2014 - 00:18
fonte

Leggi altre domande sui tag