3
\$\begingroup\$

I have the following class inheritances hierarchy:

First the abstract parent class:

<?php
abstract class SegmentParserAbstract{
/** @var ParserResult */
protected $_result;
protected $_invalidSegmentMessage;
protected $_segmentRegex;
protected $_segment;

abstract protected function createSegmentObject();
public function __construct() {
    $this->_result = new ParserResult();
}
public function setSegment($Segment) {
    $this->_segment = $Segment;
}
/**
 * Function will parse the Segment  by validating the input string. 
 * If valid it will create the Segment Object in the result array with no errors.
 * If invalid it will populate the result array with the error, but no object will be created.     
 */
public function parse() {
    if ($this->isValidSegment()) {
        $this->createSegmentObject();
    } else {
        $this->createErrorMessage();
    }
    return $this->_result;
}
protected function isValidSegment() {
    $matchesFound = preg_match($this->_segmentRegex, $this->_segment);
    return ($matchesFound > 0);
}
protected function createErrorMessage() {
    $this->_result->addError(array($this->_invalidSegmentMessage . $this->_segment));
}
}
?>

The first child:

<?php
class GsParser extends SegmentParserAbstract {
    const SUPPLIER_ID_INDEX = 2;
    const GROUP_HEADER_INDEX = 6;
    protected $_invalidSegmentMessage= 'Invalid GS segment: ';
    protected  $_segmentRegex="/^GS\*IN\*\w{2,15}\*\w{2,15}\*(\d{6}|\d{8})\*\d{4}\*\d{1,9}\*X\*\d{1,12}$/";

    protected function createSegmentObject() {
        $elements = explode('*', $this->_segment);
        $gs = new Gs();
        $gs->setSupplierId($elements[self::SUPPLIER_ID_INDEX]);
        $gs->setGroupReference($elements[self::GROUP_HEADER_INDEX]);
        $this->_result->setSegment($gs);
    }
}
?>

The second child:

<?php
class IsaParser extends SegmentParserAbstract {
    const SUPPLIER_ID_INDEX = 6;
    protected $_invalidSegmentMessage= 'Invalid ISA segment: ';
    protected $_segmentRegex="/^ISA\*00\*.{10}\*00\*.{10}\*ZZ\*.{15}\*ZZ\*.{15}\*\d{6}\*\d{4}\*U\*.{5}\*\d{9}\*[0-1]\*[PT]\*>$/";

    protected function createSegmentObject() {
        $elements = explode('*', $this->_segment);
        $isa = new Isa();
        $isa->setSupplierId($elements[self::SUPPLIER_ID_INDEX]);
        $this->_result->setSegment($isa);
    }
}
?>

Now I'll be having about nearly 16 other parsers extending the same abstract class SegmentParserAbstract with only differing in the implementation of createSegmentObject() and the number of constants (which might have different names of course).

Somehow I feel that I can remove all sub classes and only keep the parent class and make it a concrete class that can be configured to work on any Segment provided it knows its regular expression and the data that needs to be extracted and the resulting object type.

Any help?!

\$\endgroup\$
1
\$\begingroup\$

Another SE user had a related question here: When an object has different representations… what's the OO pattern?

My suggestion would be to consider the Decorator pattern to avoid the kind of repetition (methods/properties) you're dealing with in inherited classes. And to minimize the amount of code you're having to write & maintain.

With an abstract class that you're inheriting form, the method signature for createSegmentObject() is immutable. You can't easily introduce method arguments, or pass dependencies without adding additional methods into the inherited class.

With a decorator (or collection of decorators), you can continue to re-defined the behaviour of your createSegmentObject() method, which also opens up the opportunity for dependency injection as needed (so long as you don't define createSegmentObject() in your interface. It also gives you the added benefit of defining a whole different category of decorators that act upon the decorated object, e.g. persisting to a datastore.

\$\endgroup\$
1
\$\begingroup\$

My first thought is to use the Factory Pattern and supply a SegmentFactory to the SegmentParser which would cease to be abstract.

interface SegmentFactory
{
    /**
     * Returns true if $segment is valid for this factory.
     *
     * @param string $segment usually split based on a regex
     * @return bool true if valid; false otherwise
     */
    function isValid($segment);

    /**
     * Returns a new segment object from $segment.
     *
     * @param string $segment usually split based on a regex
     * @return <whatever is the parent class of Gs and Isa>
     */
    function create($segment);

    /**
     * Returns an appropriate error message when the parser cannot parse $segment.
     *
     * @param string $segment the segment that failed to be parsed
     * @return string an error message
     */
    function createErrorMessage($segment);
}

You'll have one implementation per type of segment you need to create, but it will make testing easier by splitting out the creation of segments from their parsing and the result that eventually gets built. Here's the new implementation for SegmentParser:

class SegmentParser
{
    /** @var SegmentFactory */
    private $_factory;

    /** @var ParserResult */
    private $_result;

    /** @var string */
    private $_segment;

    public function __construct(SegmentFactory $factory) {
        $this->_factory = $factory;
        $this->_result = new ParserResult();
    }

    /**
     * Function will parse the Segment  by validating the input string. 
     * If valid it will create the Segment Object in the result array with no errors.
     * If invalid it will populate the result array with the error, but no object will be created.
     *
     * Now this method takes what the factory provides and puts it into the result
     * rather than depending on the subclass having intimate knowledge of the result.
     */
    public function parse($Segment) {
        if ($this->_factory->isValid($segment)) {
            $this->_result->setSegment($this->_factory->create($segment));
        } else {
            $this->_result->addError($this->_factory->createErrorMessage($segment));
        }
        return $this->_result;
    }
}

And here's the implementation for GsSegmentFactory:

class GsSegmentFactory implements SegmentFactory
{
    const SUPPLIER_ID_INDEX = 2;
    const GROUP_HEADER_INDEX = 6;
    const REGEX = "/^GS\*IN\*\w{2,15}\*\w{2,15}\*(\d{6}|\d{8})\*\d{4}\*\d{1,9}\*X\*\d{1,12}$/";

    /**
     * Returns true if $segment is valid for this factory.
     *
     * @param string $segment usually split based on a regex
     * @return bool true if valid; false otherwise
     */
    public function isValid($segment) {
        return preg_match(self::REGEX, $segment) > 0;
    }

    /**
     * Returns a new segment object from $segment.
     *
     * @param string $segment usually split based on a regex
     * @return <whatever is the parent class of Gs and Isa>
     */
    public function create($segment) {
        $elements = explode('*', $segment);
        $gs = new Gs();
        $gs->setSupplierId($elements[self::SUPPLIER_ID_INDEX]);
        $gs->setGroupReference($elements[self::GROUP_HEADER_INDEX]);
        return $gs;
    }

    /**
     * Returns an appropriate error message when the parser cannot parse $segment.
     *
     * @param string $segment the segment that failed to be parsed
     * @return string an error message
     */
    public function createErrorMessage($segment) {
        return 'Invalid GS segment: ' . $segment;
    }
}

How is this better?

  1. Separating the parser from the factory makes testing easier. You can now test the parser with a mock factory once and each factory without a parser.
  2. The factory creates the segment or error message while only the parser deals with the result object. This is separate of concerns at its finest.
  3. Because $segment is now passed to parse you can reuse the same parser (one per factory type) and factories if you like. This also makes testing easier.
  4. You can create an abstract factory that implements isValid and createErrorMessage as you did with the original parser. Move the regex constant and error message prefix to constructor parameters if all factories work the same.

While it seems like you're just shifting from multiple parsers to multiple factories, it is generally preferable to code to interfaces rather than abstract classes. Abstract classes are great for filling out the most likely implementation of an interface which many concrete subclasses end up overriding. When you find yourself building an abstract class that has nearly all of the functionality and only implementing one template method, a factory or strategy may be a better fit.

\$\endgroup\$
  • \$\begingroup\$ Your interface lacks any kind of method scoping. By default PHP will render them in the public scope, but you should not rely on that. Explicitly defining the expected scope ensures that no method is scoped incorrectly by neglect. BTW: nice rep (1337), you should screenshot that or something. \$\endgroup\$ – mseancole Oct 19 '12 at 14:29
  • \$\begingroup\$ @mseancole - Interfaces by definition specify a public API that all implementing classes must fulfill with public methods. PHP will raise a fatal error if you attempt to implement any of the methods in the interface with anything other than public scope. And if you specify any non-public access modifier in the interface, PHP raises an error telling you it must be omitted. :) This is different from PHP assuming public access for class methods without a modifier. Public is the only option for interface methods. \$\endgroup\$ – David Harkness Oct 19 '12 at 18:21
  • \$\begingroup\$ I was under the impression that this default scoping was going to be deprecated. However, I can not find this anywhere to confirm it, so I'm wondering if this really was just a rumor. So maybe this isn't important, but, for the time being, I'm not going to go out of my way to stop adding the public keyword to my interfaces until I am sure. Better safe than sorry. Thanks again, and if I find anything either way I'll let you know. \$\endgroup\$ – mseancole Oct 19 '12 at 19:43
1
\$\begingroup\$

Edit: As David pointed out in the comments, I am completely wrong about constructors in the abstract classes. I hid the (ir)relevant section to avoid confusion and attempted to edit the rest of the post so it fit the new context. In case I missed something, Abstract classes CAN have constructors because they CAN'T be instantiated therefore any references to the init() method that may be left over should be assumed to be parent::__construct() instead. Thank you again David.

Abstract classes can not, or rather should not, be instantiated. Therefore they should never have a constructor. If you want to have a shared "constructor" there are a couple different things you can do. The easiest method is to create an init()-like function and call that in your real constructors. Another method is to extend an actual class. Since you don't seem to be trying to instantiate this class, as some who use "abstract constructors" are wont to do, then I think the first solution should be adequate, though I should add that the init() method should not be public to avoid "accidental" reinstantion.

    protected function init() {
        $this->_result = new ParserResult();
    }

Abstract classes are still classes. Therefore anything defined in it is still defined in its children. Thus, redefining properties should actually be handled in construction, because, if I'm remembering this correctly, redefining them in the children actually throws warnings. Either way, the better place is in construction.

public function __construct() {
    $this->_invalidSegmentMessage = 'Invalid GS segment: ';
    $this->_segmentRegex = "/^GS\*IN\*\w{2,15}\*\w{2,15}\*(\d{6}|\d{8})\*\d{4}\*\d{1,9}\*X\*\d{1,12}$/";

    parent::__construct();
}

So, there are a few principles being violated in your createSegmentObject() method. The first, and most obvious, is the "Don't Repeat Yourself" (DRY) principle. This can be observed by how similar all these methods are. If we take into account the Inversion of Control (IoC) Principle as well, then we can see that it is violating both of these principles. IoC is something I've never really thought about before, so I don't feel entirely comfortable explaining it. Google it if you're not sure what it is, I'll be reading up on it more myself after this post. I think you began to notice something was amiss, which is why you added it to the abstract class, you just didn't go quite far enough with it. The third principle, which is making this a little more difficult to see, is that this method also violates the Single Responsibility Principle. This method should be concerned with creating a segment, not creating AND setting. Leave that for after you call the method. If we try setting it immediately then we can't extend it.

//abstract class
protected function createSegmentObject( Segment $segment ) {
    $elements = explode('*', segment);

    $segment->setSupplierId( $elements[ self::SUPPLIER_ID_INDEX ] );
    $this->segment = $segment;
    $this->elements = $elements;
}

//extending class
protected function createSegmentObject( Segment $segment ) {
    parent::createSegmentObject( $segment );

    $this->segment->setGroupReference( $this->elements[ self::GROUP_HEADER_INDEX ] );
}

//usage
if ($this->isValidSegment()) {
    $this->createSegmentObject();
    $this->_result->setSegment( $this->segment );
}

In case you are curious, createSegmentObject( Segment $segment ) is assuming that your "segments" are using a common interface or abstract class and that it is called "Segment". If you are not using one of these, then I would seriously think about doing so. Assuming for now that you are, then we can declare the parent class as the common denominator and type hint our parameters for it. This will ensure that any object passed to the above method must be a type of Segment and will be able to produce the expected results every time, else PHP will throw errors.

So, we've just eliminated most of the need for multiple classes. The only thing holding us back are these unique properties and the occasional extensions. The interesting thing about these classes is that the properties are very structured and unique. So what do we do when we have a common data structure that needs to be saved and reused? If we create a database with each row denoting a segment and each field a property, then we can construct our segments by calling that specific row in the database. Of course, we should inject these parameters into our constructor to avoid our class becoming dependent. I'll leave the database implementation to you, but here's the constructor:

public function __construct( $id, $group, $type, $regex ) {
    $this->_supplierIdIndex = $id;
    $this->_groupHeaderIndex = $group;
    $this->_invalidSegmentMessage = "Invalid $type segment: ";
    $this->_segmentRegex = $regex;

    parent::_construct();
}

That leaves us with just extensions holding us back now. We could create an elaborate comparison table and pick and choose when to use what, or we can automate the process and use a factory pattern. Again, I'm going to leave this up to you, for multiple reasons, but the main one being that I haven't played with factories yet and wouldn't be sure where to begin. Besides, without the rest of the subclasses I couldn't begin.

I believe some of this is what Saul linked to, but I just skimmed over it. I did spy something about a factory in there, so maybe much of this is the same, but it lacked a lot of the explanations that I felt were necessary. Hopefully this helps.

\$\endgroup\$
  • \$\begingroup\$ I felt bad for leaving IoC so vague in my post, so here's a good resource I was looking at yesterday: stackoverflow.com/questions/3058/what-is-inversion-of-control/… \$\endgroup\$ – mseancole Oct 18 '12 at 16:51
  • 1
    \$\begingroup\$ The class is abstract so it cannot be instantiated directly. There's no need to move the code you'd normally have in a constructor into a separate init method. The only difference is the name of the method which adds nothing. Use a constructor and have the children override it if necessary. \$\endgroup\$ – David Harkness Oct 19 '12 at 3:49
  • \$\begingroup\$ @DavidHarkness: You are absolutely correct. I have no idea how I got it into my head that abstract classes could still be instantiated, but I just spent the last half hour trying to prove this and couldn't. I'll edit my post to reflect this, thank you for putting me straight. \$\endgroup\$ – mseancole Oct 19 '12 at 13:40

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.