3

I have hardware I communicate with via TCP. This hardware accepts ~40 different commands/requests with about 20 different responses. I've created a HardwareProxy class which has a TcpClient to send and receive data. I didn't like the idea of having 40 different methods to send the commands/requests, so I started down the path of having a single SendCommand method which takes an ICommand and returns an IResponse, this results in 40 different SpecificCommand classes. The problem is this requires semantic coupling, i.e. the method that invokes SendCommand receives an IResponse which it has to downcast to SpecificResponse, I use a future map which I believe ensures the appropriate SpecificResponse, but I get the impression this code smells. Besides the semantic coupling, ICommand and IResponse are essentially empty abstract classes (Marker Interfaces) and this seems suspicious to me.

If I go with the 40 methods I don't think I have broken the single responisbility principle as the responsibility of the HardwareProxy class is to act as the hardware, which has all of these commands. This route is just ugly, plus I'd like to have Asynchronous versions, so there'd be about 80 methods.

Is it better to bite the bullet and have a large class, accept the coupling and MarkerInterfaces for a smaller soultuion, or am I missing a better way?

Thanks.

Possible Solution

Based on DXM's response this is what I came up with:

struct Proxy
{
   template<class C, class R>
   ICommand<C, R>* CreateCommand()
   {
      return new C(this);
   }

   void Send(std::string s)
   {
      //tcp send here for now print command name
      cout << s << endl;
   }
};

template<class Derived>
struct IResponse
{};

struct DerivedResponse1 : IResponse<DerivedResponse1>
{
   std::string GetValue()
   {
      return "DerivedResponse1";
   }
};

template<class Derived, class Response>
struct ICommand
{
protected:
   Proxy* pProxy;
public:
   Response* Send()
   {
      std::string msg = "Sent: " + static_cast<Derived*> (this)->GetName();
      pProxy->Send(msg);
      //wait for future here
      Response* pResponse = new Response; //replace with future value
      return pResponse;
   }
};

struct DerivedCommand1 : public ICommand<DerivedCommand1, DerivedResponse1>
{
   DerivedCommand1(Proxy* pProxy)
   {
      this->pProxy = pProxy;
   }

   std::string GetName()
   {
       return "DerivedCommand1";
   }
};

int main()
{
   Proxy proxy;

   ICommand<DerivedCommand1, DerivedResponse1>* pDerivedCommand = 
           proxy.CreateCommand<DerivedCommand1, DerivedResponse1>();
   DerivedResponse1* pDerivedResponse = pDerivedCommand->Send();
   cout << "Received :" << pDerivedResponse->GetValue() << endl;

   return 0;
}

How does that look? It seems like I may not even need the IResponse interface, we'll have to see when I implment it. I realize there is a lot of important stuff missing, like the futures, but I wanted to get the template stuff down first. What do you think of passing in the proxy to the command? Also what about having the send in the ICommand as opposed to writting it for every command? Am I violating any other OO principles? Thanks.

2

I think your move from 40 methods in one class to 40 command classes was done in the right direction. I would continue moving even more in that direction...

Instead having one sending entry point in your main HardwareProxy class, have each one of your concrete classes implement send() which would return a concrete response object. Under the hood the command classes can derive from the common command base class, which would contain a reference back to the HardwareProxy (or maybe TcpClient is enough).

Another good thing about c++ is that you can define a clean, type-safe public interface with the help of templates:

proxy = new HardwareProxy();
proxy->Connect(...);

SpecificCommand* command = proxy.CreateCommand<SpecificCommand>();
SpecificResponse* response = command->send(...);


Update: Your last idea isn't exactly what I had in mind, as you could probably tell if you compare my client example above with your main function

I don't think there's any value in defining generic ICommand class, especially since as soon as you make it a ICommand<> template, you lose polymorphism anyway. Your client code should be able to work directly with your SpecificCommand/SpecificResponse classes. Presumably each command/response would have different data going in and coming out, so no point of putting that part into a generic interface.

However, if you have 40 different unique commands/responses, they will have something in common, the way they package/serialize/deserialize the data in order to prepare it for the TCP comms. So what I would do is have a BaseCommand/BaseResponse classes (still not templates) that would handle all common work and potentially be responsible of talking to the TCPClient object. Base classes would also allow your proxy to treat all commands in a uniform way in case you need things like being able to track which commands is outstanding or being able to iterate through all of them and gracefully clean them up when TCP connection drops.

While I love c++ templates and do use them rather freely, one word of advice I would give you is avoid them unless you actually need them. In your example, you are using a template parameter to do a pointer cast in order to access derived class' member function. You could just as easily declare GetName() to be virtual. The reason why I suggested CreateCommand<>() template function is only because the code would look cleaner because otherwise, it would have to return BaseCommand type and your client code would have to do yet another pointer cast. But the actual underlying design that I'm thinking could easily be done with no templates.

  • Thanks. I had originally considered this but didn't like having the commands know about the Proxy/TcpClient, that seemed inverted to me, but that is probably less evil than the coupling I had going on. I've updated the post based on your input, is that what you had in mind? – zrp Oct 31 '13 at 23:37
  • Sorry, perhaps I did get a bit carried away with the templates. I thought my main looked fairly similiar to your code. The thing is I haven't figured out how to do the Tcp stuff in the base class without the curiously recurring template pattern. I'll work on it some more and see if I can bring it more into line with what you had in mind. I appreciate the help, when I get it working I'll check your response as the answer. – zrp Nov 1 '13 at 15:07
  • 1
    @zrp: so you are implying that IF you don't get it working, I won't get my 15 points? :)... Don't worry too much about the figuring out part. You can start with 2 or 3 commands and just write the code to get it working, and if it's simpler, do it all in deriving classes. Once some of it is written, you should see a pattern emerging. Some code will be unique to the specific command, but a bunch of code you'll probably copy/paste from one command to another. So then take a refactoring pass and put the common code into base... I'm guessing that's how it'll go, but maybe you won't have... – DXM Nov 1 '13 at 15:11
  • ... that much common code. Or maybe you'll end up putting it in a different class. But one thing to keep in mind with OOP (and c++ templates) is while thinking about design and patterns is great, a lot of times to come up with the right design, you should just write some code and then refactor it. – DXM Nov 1 '13 at 15:14
1

Personally, I am fine with the 40 functions. It doesn't sound as though you'll want som subset/interface of the proxy, and tcp commnication isn't a great candidate for unit testing.

Not everything needs to be OOP.

That said, I think it is foolish to try and tie that'll of the commands into a single interface. Let's face it - not all commands take the same parameters, and users will not want to iterate through arbitrary commands and execute them. They are going to request a specific command (likely by specific command interface) which implicitly contains its payload and response.

  • I agree not everything has to be OOP, I mostly do embedded programming, sometimes OO is very difficult with DSP's. That said orginally I thought this was the perfect place for inheritance, but the coupling and marker interfaces made me suspicious. Can you elaborate on this: "I think it is foolish to try and tie that'll of the commands into a single interface"? I agree about iterating through arbitrary commands, but it seems like DXM's response avoids that, do you disagree? Thanks. – zrp Nov 1 '13 at 15:17
  • @zrp - It does, but your edit does not. – Telastyn Nov 1 '13 at 17:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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