Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function to reindex all segments in a message #125

Merged
merged 3 commits into from
Dec 22, 2024

Conversation

jay-knight
Copy link
Contributor

This adds a new function to the Message class to re-index all of the segments in a message. When we're reading and building multiple messages, the index/sequence ids can be added in weird and unpredictable ways because of the static/global $setId variables.

This solution lets you add any segments you need and then step through and re-index all of the segments so that each segment types starts with 1 and increases by 1 for each segment of that type.

I think a better solution might be to keep up with the segment ids in the Message class (as an instance variable, instead of static variables on each segment class) so that as you add segments they can be set/incremented if that option is set. And multiple messages wouldn't interfere with each other.

@jay-knight jay-knight force-pushed the reindex-message-segments branch from 948cd58 to 364cda7 Compare December 6, 2024 18:13
@senaranya
Copy link
Owner

Thanks for adding this. Can you please add it in README.md under the section "Handling segments and fields"?

@jay-knight
Copy link
Contributor Author

jay-knight commented Dec 16, 2024

I've added a bit about the new function in README.md.

I also changed the function to only index segments whose class has a setID() function. Some other segments from the standard don't have a Set ID, so this should be more future compatible if those segments get implemented in this library.

Do you think it would be better to have an interface (HasSetID?) that classes with this field could implement and check if each class implements that interface:

namespace app/HL7;

interface HasSetID {
    public function setID(int $value, int $position = 1);
    public function getID(int $position = 1);
}
class PID extends Segment implements HasSetID {
    [...]
}
        foreach ($this->segments as $segment) {
            if ($segment instanceof HasSetID) { // <-- instead of method_exists()
                if (!array_key_exists($segment->getName(), $indexes)) {
                    $indexes[$segment->getName()] = 1;
                }
                $segment->setId($indexes[$segment->getName()]++);
            }
        }

@jay-knight jay-knight force-pushed the reindex-message-segments branch from 52880e0 to f7e7963 Compare December 16, 2024 17:33
@senaranya
Copy link
Owner

Thank you for updating README. I think method_exists() is fine for now, it does the job with minimal changes. We can later refactor it to interface if need arises

@senaranya senaranya self-assigned this Dec 22, 2024
@senaranya senaranya merged commit 98c0053 into senaranya:master Dec 22, 2024
3 checks passed
@shadowhand
Copy link

shadowhand commented Dec 23, 2024

Do you think it would be better to have an interface (HasSetID?) that classes with this field could implement and check if each class implements that interface

The interface HasSetId would be a great name for a trait that implements the interface, but not a good name for an interface. I would suggest SetIdentifier or Identified:

if ($segment instanceof Identified) { /* ... */ }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants