NotionCommotion Posted February 18, 2018 Share Posted February 18, 2018 In the ClientCollections class, I have the following: $this->clientList[$connStream]->client->callbackCollections->get($rpcId)->executeSourceResponse($data); $this->clientList[$connStream]->client is the Client application tied to a given connection, and I think it is good as is. callbackCollections is the list of callbacks associated with the instance of a client, and has a get() method which returns a given callback. All callbacks have the executeSourceResponse() method which performs the applicable task. Should I restructure ClientCollections so that this level need not know all the gory details such as the following? $this->clientList[$connStream]->client->executeSourceResponse($guid, $data); And then Client::executeSourceResponse($guid, $data) does: $this->callbackCollections->executeSourceResponse($guid, $data); And then CallbackCollections::executeSourceResponse($guid, $data) does: $this->get($guid)->executeSourceResponse($data); But then I have these three levels of executeSourceResponse() methods which gets confusing. Sure, I can rename them, but I don't know how that would make things less confusing. What is considered "good practice" for something like this? Related to this, ClientCollections, Client, and CallbackCollections are currently in the same namespace and may throw a JsonRpcException exception which is caught by ClientCollections. There are a bunch of different versions of callbacks which are returned by CallbackCollections::get(), so I put them in their own namespace to keep them organized. I am debating on allowing these callbacks to be able to also throw an \JsonRpcException exception, or instead have them through their own exception and do something like: try { $this->get($guid)->executeSourceResponse($data); } catch(Callbacks\CallbackException $e) { throw new JsonRpcException($e->getMessage(), $e->getCode()); } I don't know if this is really getting me anything other than potentially more flexibility in the future. Quote Link to comment Share on other sites More sharing options...
ignace Posted February 19, 2018 Share Posted February 19, 2018 (edited) I would go with something like: <?php $client = $this->clientList[$connStream]->Client; $client->executeSourceResponse($guid, $data); // Where Client::executeSourceResponse would do: $this->callbackCollections->get($guid)->executeSourceResponse($data); It cleanly separates concerns. Whatever class has the ClientList is already aware of ClientList and Client. It shouldn't be made aware of further internals of the Client. This makes sure that if in the future you would re-structure Client, no other classes would need to be updated. Edited February 19, 2018 by ignace Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted February 19, 2018 Author Share Posted February 19, 2018 Thank you ignace, Sometimes when not certain in the right approach, I get myself in a loop and accomplish nothing. 1 Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.