patternphpMinor
When an object has different representations... what's the OO pattern?
Viewed 0 times
thewhathasdifferentrepresentationswhenobjectpattern
Problem
I've an
Message itself may have different representations (query string, resource string or an instance of
```
interface TransportInterface
{
public function executeTransport(AbstractMessage $message, array &$fails = array());
}
class HttpTransport implements TransportInterface
{
/**
* @var \Guzzle\Service\Client
*/
private $client;
/**
* @var Converter\MessageConverterInterface
*/
private $converter;
public function __construct(Client $client, MessageConverterInterface $converter)
{
$this->client = $client;
$this->converter = $converter;
}
public function executeTransport(AbstractMessage $message, array &$fails = array())
{
$representation = $this->converter->convert($message);
/ ... /
}
}
class RestTransport implements TransportInterface
{
/**
* @var \Guzzle\Service\Client
*/
private $client;
/**
* @var Converter\MessageConverterInterface
*/
private $converter;
public function __construct(Client $client, MessageConverterInterface $converter)
{
$this->client = $client;
$this->converter = $converter;
}
public function executeTransport(AbstractMessage $message, array &$fails = array())
{
$representation = $this->converter->convert($message);
/ ... /
}
}
class MailerTransport implements TransportInterface
{
/**
* @var \Swift_Mailer
*/
private $mailer;
/**
* @var Converter\MessageConverterInterface
*/
AbstractMessage object (a hierarchy, actually). It represents a small text message and can be sent using different transport methods: HTTP, REST and a "mailer" transport. Each transport relies on an external library for executing the transport itself, injected using a DI container.Message itself may have different representations (query string, resource string or an instance of
\Swift_Message), based on the transport used. A transport should use the more appropriate representation, again injected using constructor injection.```
interface TransportInterface
{
public function executeTransport(AbstractMessage $message, array &$fails = array());
}
class HttpTransport implements TransportInterface
{
/**
* @var \Guzzle\Service\Client
*/
private $client;
/**
* @var Converter\MessageConverterInterface
*/
private $converter;
public function __construct(Client $client, MessageConverterInterface $converter)
{
$this->client = $client;
$this->converter = $converter;
}
public function executeTransport(AbstractMessage $message, array &$fails = array())
{
$representation = $this->converter->convert($message);
/ ... /
}
}
class RestTransport implements TransportInterface
{
/**
* @var \Guzzle\Service\Client
*/
private $client;
/**
* @var Converter\MessageConverterInterface
*/
private $converter;
public function __construct(Client $client, MessageConverterInterface $converter)
{
$this->client = $client;
$this->converter = $converter;
}
public function executeTransport(AbstractMessage $message, array &$fails = array())
{
$representation = $this->converter->convert($message);
/ ... /
}
}
class MailerTransport implements TransportInterface
{
/**
* @var \Swift_Mailer
*/
private $mailer;
/**
* @var Converter\MessageConverterInterface
*/
Solution
I would use an abstract class instead of, or in addition to, an interface. You have quite a bit of repetition that could be removed by doing so. The only problem then would be that you would have to use protected instead of private properties and methods, at least for those that you need to share between the abstract class and inheriting class, but I don't think that should make too much of a difference. Remember: You can't create an instance of an abstract class, so you have to use some other "initiator" and call that in each constructor.
You might also want to be careful with referencing. It can sometimes be hard to determine where a variable was changed if you don't know that it was assigned by reference somewhere. This is entirely your choice, just a suggestion. I would, however, like to point out that if this parameter is referenced, then setting a default value to it is kind of pointless. If the parameter has not been set, then that value can't be used and should therefore not be used at all to avoid the overhead.
I've never heard of, nor could find a library design pattern, so I can't be sure. As for the name of the pattern you are using? I honestly don't know. But it doesn't matter. Functional code is much better than any code that adheres to a design pattern just for the sake of doing so. Many times I see programmers attempting to force their code into a design pattern, and some times its not even the right pattern. Design patterns are not necessary at this stage. Code for functionality. Once you are comfortable with OOP, then you can start worrying about design patterns and how and when to apply them.
Now, as to whether this is good OOP or not; It appears to be fine, however, this code is incomplete, so it is hard to tell. The concepts seem fine, but the implementation is unsure. You are using encapsulation, inheritance, and a sort of polymorphism. Those are some of the main points of OOP, and you seem to grasp them fairly well, though you could probably stand to look into polymorphism a bit more. You are also using interfaces and dependency injection. These are also important concepts. A couple more points that might help is to ensure you are following the Single Responsibility Principle (can't be sure) and the "Don't Repeat Yourself" (DRY) principle. As I pointed out above, you could use some help with DRY.
Finally, your code samples could have easily been reduced to a single interface and class with a note that the other related classes only differed in the commented area. The other classes, except that last, are redundant, whereas that last is unnecessary in this context. If you would have submitted complete classes, this would not be the case. The second interface was slightly relevant, but was also unnecessary.
I hope this helps!
EDIT
Example implementation of above code:
Per comment, here's how to do it with separate classes for each type:
And you'd do pretty much
abstract class AbstractTransport {
protected
/** @var \Guzzle\Service\Client */
$client,
/** @var \Swift_Mailer */
$mailer,
/** @var Converter\MessageConverterInterface */
$converter
;
protected function initClient( Client $client, MessageConverterInterface $converter ) {
$this->client = $client;
$this->converter = $converter;
}
protected function initMailer( Swift_Mailer $mailer, MessageConverterInterface $converter ) {
$this->mailer = $mailer;
$this->converter = $converter;
}
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
$representation = $this->converter->convert( $message );
}
}You might also want to be careful with referencing. It can sometimes be hard to determine where a variable was changed if you don't know that it was assigned by reference somewhere. This is entirely your choice, just a suggestion. I would, however, like to point out that if this parameter is referenced, then setting a default value to it is kind of pointless. If the parameter has not been set, then that value can't be used and should therefore not be used at all to avoid the overhead.
I've never heard of, nor could find a library design pattern, so I can't be sure. As for the name of the pattern you are using? I honestly don't know. But it doesn't matter. Functional code is much better than any code that adheres to a design pattern just for the sake of doing so. Many times I see programmers attempting to force their code into a design pattern, and some times its not even the right pattern. Design patterns are not necessary at this stage. Code for functionality. Once you are comfortable with OOP, then you can start worrying about design patterns and how and when to apply them.
Now, as to whether this is good OOP or not; It appears to be fine, however, this code is incomplete, so it is hard to tell. The concepts seem fine, but the implementation is unsure. You are using encapsulation, inheritance, and a sort of polymorphism. Those are some of the main points of OOP, and you seem to grasp them fairly well, though you could probably stand to look into polymorphism a bit more. You are also using interfaces and dependency injection. These are also important concepts. A couple more points that might help is to ensure you are following the Single Responsibility Principle (can't be sure) and the "Don't Repeat Yourself" (DRY) principle. As I pointed out above, you could use some help with DRY.
Finally, your code samples could have easily been reduced to a single interface and class with a note that the other related classes only differed in the commented area. The other classes, except that last, are redundant, whereas that last is unnecessary in this context. If you would have submitted complete classes, this would not be the case. The second interface was slightly relevant, but was also unnecessary.
I hope this helps!
EDIT
Example implementation of above code:
class RestTransport extends AbstractTransport {
public function __construct( $client, $converter ) {
$this->initClient( $client, $converter );
}
}Per comment, here's how to do it with separate classes for each type:
//only worth it if both client and mailer implementations share more than this
abstract class AbstractTransport implements TransportInterface {
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
$representation = $this->converter->convert( $message );
}
}
class Client extends AbstractTransport {
public function __construct( Client $client, MessageConverterInterface $converter ) {
$this->client = $client;
$this->converter = $converter;
}
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
parent::executeTransport( $message, $fails );
/*
assuming there will be shared aspects unique to clients
you can use polymorphism to further extend method
same in the mailer
otherwise it is unnecessary to redefine here
*/
}
}
class RestTransport extends Client {
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
parent::executeTransport( $message, $fails );
/* ... */
}
}And you'd do pretty much
Code Snippets
abstract class AbstractTransport {
protected
/** @var \Guzzle\Service\Client */
$client,
/** @var \Swift_Mailer */
$mailer,
/** @var Converter\MessageConverterInterface */
$converter
;
protected function initClient( Client $client, MessageConverterInterface $converter ) {
$this->client = $client;
$this->converter = $converter;
}
protected function initMailer( Swift_Mailer $mailer, MessageConverterInterface $converter ) {
$this->mailer = $mailer;
$this->converter = $converter;
}
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
$representation = $this->converter->convert( $message );
}
}class RestTransport extends AbstractTransport {
public function __construct( $client, $converter ) {
$this->initClient( $client, $converter );
}
}//only worth it if both client and mailer implementations share more than this
abstract class AbstractTransport implements TransportInterface {
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
$representation = $this->converter->convert( $message );
}
}
class Client extends AbstractTransport {
public function __construct( Client $client, MessageConverterInterface $converter ) {
$this->client = $client;
$this->converter = $converter;
}
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
parent::executeTransport( $message, $fails );
/*
assuming there will be shared aspects unique to clients
you can use polymorphism to further extend method
same in the mailer
otherwise it is unnecessary to redefine here
*/
}
}
class RestTransport extends Client {
public function executeTransport( AbstractMessage $message, array &$fails = array() ) {
parent::executeTransport( $message, $fails );
/* ... */
}
}Context
StackExchange Code Review Q#15899, answer score: 5
Revisions (0)
No revisions yet.