Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am designing a program which encodes / decodes protocol messages to/from various bits of hardware.

//base protocol class - interface only
//ref_type is a smart pointers class - not really so relevant to my question
class BaseMessage : public ref_type
{
public:
    virtual ~BaseMessage() {}
    virtual const char* ProtocolName() const = 0;
    virtual bool Encode() = 0;
    virtual bool Decode() = 0;
    //for binary protocols
    virtual void getMessage(unsigned char*& serialised, size_t& length) {}; 
    //for text/xml protocols
    virtual void getMessage(std::string& serialised) {}
};

A specific protocol might then be implemented like this:

class ABCMessage : public BaseMessage {
public: 
   ABCMessage(ABC* msg) : m_msg(msg), m_data(0), m_length(0) {}
   ABCMessage(unsigned char* data, int length) : m_length(length), m_msg(0) 
   {
      m_data = new unsigned char[length]();
      memcpy(m_data, data, length);
   }
   virtual ~ABCMessage();
   virtual const char* ProtocolName() const;
   virtual bool Encode();
   virtual bool Decode();

   virtual void getMessage(unsigned char*& serialised, size_t& length);

protected:
   ABC* m_msg;
   unsigned char* m_data;
   size_t m_length;
};

Basic idea is for users of these classes to create derived polymorphic protocol specific messages. There is no protocol type in the interface, it is all hidden behind the scenes. But the serialised messages can be retrieved with the getMessage functions.

Does this look a logical approach? Any suggestions?

share|improve this question

migrated from stackoverflow.com Nov 23 '12 at 15:07

This question came from our site for professional and enthusiast programmers.

    
It's perhaps not clear who allocates, owns, handles memory in calls to getMessage and constructor. Usable, but error-prone. –  ActiveTrayPrntrTagDataStrDrvr Nov 23 '12 at 14:52
    
Decode() should probably return 'size_t'. If the intention is to return -1 in case of error, then it should be documented. But better to throw an exception (and document it, too). –  piokuc Nov 23 '12 at 14:56
    
@BjörnPollex - how can I migrate it? –  arcomber Nov 23 '12 at 15:00
    
getMessage() could return a bool, and ProtocolName() could return a std::string, or const std:string&. –  user1158692 Nov 23 '12 at 15:00
    
@user619818: Flag it. –  John Dibling Nov 23 '12 at 15:00

2 Answers 2

OK. I doubt it is irrelevant.

//ref_type is a smart pointers class - not really so relevant to my question

Also smart pointers are very hard to get perfectly correct. The standard ones took years and thousands of people looking at them to get correct. So It is usually preferable to use one of the standard smart pointers rather than a home grown one (especially one that is not being reviewed).

So who owns this pointer?

    virtual const char* ProtocolName() const = 0;

There are no ownership semantics associated with pointers. So After I call this I am not sure who is responsible for deleting it. Use a std::string internally and return a const reference to the string.

Not sure why you would want two methods that look like they do the same thing.

    //for binary protocols
    virtual void getMessage(unsigned char*& serialised, size_t& length) {}; 
    //for text/xml protocols
    virtual void getMessage(std::string& serialised) {}

Both are going to be inefficient as you are copying across multiple buffers (input stream buffer into this buffer which is then returned and read again to build an object). What you really want is a getMessage() method that returns a fully constructed object from the stream.

This calss:

class ABCMessage : public BaseMessage {

Is totally broken as it contains an owned RAW pointer but does not implement the rule of three.

share|improve this answer
    
As far as I know const char * is often used to denote fact that no one should take owner ship on that poitner (i.e. someone who made that from char * is owner). –  ony Dec 24 '12 at 12:25
    
@ony: No. const char* is just the same as chat const* and indicates absolutely no ownership. The only thing it indicates is that the content can not be modified (this has nothing to do with ownership). This is why C++ has std::string. You can return by value (thus returning ownership) or return by reference (indicating that ownership is retained by the source object). –  Loki Astari Dec 24 '12 at 16:26
    
I said "is often used to denote" but not "it denotes". I refer to some guidelines in code writing. Often fact that you can't modify content results in inability to destroy object correctly (ex. decrement amount of refs etc). –  ony Dec 24 '12 at 21:12
    
@ony: No. It is never used this way in C++. In C or "C with struct" (people who write C who think they write C++) may do this. But in C++ this concept was moved to the rubish pile of bad techniques over a decade ago. In good program ownership is explicitly expressed by the objects. –  Loki Astari Dec 24 '12 at 21:34
    
anyway, I hope you've got idea. Even that this thing never used this way and in the same time it moved to the rubish pile of bad techniques in favor of smart pointers. –  ony Dec 25 '12 at 8:57

There is very good approach which works almost always: "Look for existing solutions". Consider looking into boost serialization as model for your implementation. I.e. I'd have some buffered streams to serialize in/out content and expose methods that can serialize to that stream and from it (or construct from it).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.