I have hardware I communicate with via TCP. This hardware accepts ~40 different

Question

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.

Explanation / Answer

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.

Related Questions

Navigate

• Browse (All)
• Browse I
• Subjects

Integrity-first tutoring: explanations and feedback only — we do not complete graded work. Learn more.