Using constants for field positions

[pluk77]

Would you be open to receiving a PR for adding constants to the segments (and perhaps drop the $position argument completely)? Hard-coding the same value in setter and getters seems a bit odd and by using constants you have additional benefits in your IDE and documentation.

For simple segments, one could even completely bypass the getter and setters without the risk of using the wrong position:

$orc = new ORC();
$orc->setField(ORC::ORDER_CONTROL, $value);

I propose to change the segments like this:

class ORC extends Segment
{
    const ORDER_CONTROL = 1;
    const PLACE_ORDER_NUMBER = 2;
    
    public function __construct(array $fields = null)
    {
        parent::__construct('ORC', $fields);
    }

    public function setOrderControl($value)
    {
        return $this->setField(self::ORDER_CONTROL, $value);
    }

    public function setPlacerOrderNumber($value)
    {
        return $this->setField(self::PLACE_ORDER_NUMBER, $value);
    }

    public function getOrderControl()
    {
        return $this->getField(self::ORDER_CONTROL);
    }

    public function getPlacerOrderNumber()
    {
        return $this->getField(self::PLACE_ORDER_NUMBER);
    }
}

or like this

class ORC extends Segment
{
    const ORDER_CONTROL = 1;
    const PLACE_ORDER_NUMBER = 2;
    
    public function __construct(array $fields = null)
    {
        parent::__construct('ORC', $fields);
    }

    public function setOrderControl($value, int $position = self::ORDER_CONTROL)
    {
        return $this->setField($position, $value);
    }

    public function setPlacerOrderNumber($value, int $position = self::PLACE_ORDER_NUMBER)
    {
        return $this->setField($position, $value);
    }

    public function getOrderControl(int $position = self::ORDER_CONTROL)
    {
        return $this->getField($position);
    }

    public function getPlacerOrderNumber(int $position = self::PLACE_ORDER_NUMBER)
    {
        return $this->getField($position);
    }
}

[senaranya]

We may require the $position argument in certain cases, so first option can't be done.

I wouldn't mind going for the second one, but for segments with a lot of fields, the number of constants would grow and make the class quite unwieldy, which are already quite large as it is.

[pluk77]

The problem I see it that changing the position argument may not have the desired effect in all cases.

For example, if the position argument in MSH::setMessageType() is changed, the ACK message will not be functioning correctly anymore as the position of Message Type is hard-coded in there to 9:

$msh->setField(9, 'ACK');

To me this means the user is lead to believe all positions can be changed, yet if that happens other parts of the library will break.

If certain implementations use different positions for different fields, it should be set in a single place so all references to this position then use the correct number.

Anyway, I will prepare a PR which makes use of the constants, but leave the argument in the getters and setters.

I hope you like it and can consider it for a merge.

[senaranya]

Please go ahead.

While I agree in MSH changing positions of some fields does not make sense, other segments may be slightly flexible. The library gives the user flexibility while keeping sane defaults. So if the users want to override something, they can, but only by being explicit about it.