This is a great idea in theory, but is sometime difficult to implement in practice. I duplicated logic which “understands” the structure of the codes table because I couldn’t easily decipher how to reuse the logic you implemented. Which incidentally also does “repeat itself.” You repeat the table names and column names and their mappings based on the external id as “nested if then statements” for code_set_search and lookup_code_descriptions.
IMHO this is a less than ideal approach, as one is required to update both functions instead of updating metadata “descriptions” of the tables when needed.
I have also even extended the paradigm, by including metadata for the existing “codes” table such that both it and the “external tables” are treated in much the same way.
So now the logic of what table and columns to use can be accomplished quickly as a single “array lookup” and then everything else is handled identically.
My search code can also be updated to not “special case” external_table==0;
These are pretty significant changes to your code, so I’m expecting resistance, but I hope others will chime in on their opinion of my approach to describe the table structures in an array, which I think promotes re-use far better than the current approach.
My new logic for lookup_code_descriptions is incomplete as it doesn’t fully duplicate how the original version handles “revisions” but it will be pretty easy to address that if people think my approach is worth pursuing.
The paradigms I stated were:
1. The search function is centralized to support any code type.
2. When an external data set is used, note that the codes table is still utilized to hold modifications of the codes (for example, if a user disables a code, it is recorded in the codes table and not the external tables).
3. It was built in a way to easily add new code types or for new features to support all code types. For example, see here where a developer plans to add internationalization support to the SNOMED data sets: http://sourceforge.net/tracker/?func=detail&aid=3599361&group_id=60081&atid=1245239
(note how the current mechanism in the codebase allows the very straightforward support of this with minimal effort in one spot)
If you can improve/clarify the feature while maintaining the paradigms (and current functionality), then please feel free to do so. The one other thing to add would be to allow the flexibility of not needing to follow the metadata and allow creation of a custom query within the function(s) (in the cases where a developer might be bringing a code set with a more complicated table(s) structure (ie. something similar to the rxnorm tables, for example).
BTW:
The last thing I stated is not just theoretical. There will likely be a RXNORM code passing through this function at some point possible for Allergy/Medication list items in the future, which is why having the option to not follow the metadata will be useful (plan to actually embed a modularized function for this).
I modified the central code_set_search() function to support Kevin’s sequential search method and placed a wrapper for it in the custom/code_types.inc.php script (the sequential_code_set_search() function). It’s testing well for all code types and I have it even being used in the code finder pop-up: http://github.com/bradymiller/openemr/commit/5cfdebb0e3f5b55a3c021e974c22038c72b3e508
Note that it maintains the centrality of the code type searching mechanism while continuing to allow very flexible addition of new custom code types in a non-redundant fashion.
Please feel free to review this code. And Kevin, again, please feel free to improve this (via your metadata method), in keeping with the paradigms discussed above.
Brady,
This seems like a “turf war” to me. You just seem to want to own the code search functionality. Trying to have the “code_set_search” function handle “every situation” is not a good approach. The above commit also uses “copy and paste” programming style which is just awful. I see this block of code pasted nearly identically multiple times. If somebody needed to fix something that was “common” they’d have to do it in every one of those blocks. You added about a hundred lines of code. Functions that big a *really* hard for people to understand.
else if ($mode == "code") {
$query .= "WHERE (c.code LIKE ?) ";
array_push($sql_bind_array,$search_term."%");
}
else if ($mode == "description") {
if (is_array($description_keywords)) {
// Multiple description keywords
$query .= "WHERE ( 1=1 ";
foreach($description_keywords as $keyword) {
$query .=" AND c.code_text LIKE ? ";
array_push($sql_bind_array,"%".$keyword."%");
}
$query .= ") ";
}
else {
// Only one description keyword
$query .= "WHERE (c.code_text LIKE ?) ";
array_push($sql_bind_array,"%".$description_keywords."%");
}
}
else { // $mode == "default"
As an aside, I think that trying to shoe horn in RXNORM search functionality into this code_set_search is a bad idea. The main reason being is that drug codes are used very differently than these other code set. Using the relationship data in the RXNORM tables between the different drug concepts to filter results is going to be critical to successful integration of that data. As an example. search rxnconso for “STR like %hydrochlorothiazide%”. there are more than four thousand entries.
My approach would have allowed the change for spanish diagnoses in one line by updating one piece of metadata and defining a new type.
define_external_table($code_external_tables,2,'sct_concepts','ConceptId','FullySpecifiedName',' ConceptStatus=0 and FullySpecifiedName LIKE \'%(disorder)\'');
Changing the external_id to something new, and changing “disorder” here to the spanish translation would have been all that was needed.
Your approach means copying and pasting all the logic previous logic and duplicating it AGAIN.
I refuse to touch custom/code_types_inc at this point, because I foresee a continued battle trying to get your approval, and frankly it’s not worth my time. You have “won.” I concede and will" do it your way."
There is an obvious misunderstanding here or you are simply creating your own reality. Your code did not even touch the code_set_search() function!
I assumed you were going to implement your metadata method into this function, so I spent my resources in order to integrate your sequential search method into it before you did so. Did you even plan on integrating your metadata function into the code_set_search() function. If not, then how was your metadata method going to centralize things? My above commit maintains:
1. Robustness
2. Modularity
3. Centralization
4. Expandability
I completely agree that it is “wordy” and that breaking that function into metadata would be very useful, but I am not going to do it. I have seriously done enough don’t you think, considering I was the one 1)to centralize all of this stuff, 2)support all code types and then 3) even integrate your search algorithm into it. You can do it if you feel so strongly; you have my complete support.
Regarding rxnorm, just providing an example why to keep it flexible (as you elude to, it’s not a simple query in that case and involves table joining and multiple fields) and who knows, someday these may not even be queries, but instead web service requests…
Regarding the process of code reviews, it may make sense you to contact myself and/or other contributors offline if need guidance.
Looking forward to your next revision,
-brady OpenEMR
I assumed you were going to implement your metadata method into this function, so I spent my resources in order to integrate your sequential search method into it before you did so.
Why did you bother though? What you did just makes trying to get the metadata approach into code_set_search harder. It would have been much easier to integrate my search method *after* I had done the refactor.
Besides, I don’t WANT my search algorithm integrated into code_set_search. The approach is different enough from code_set_search that adding a flag/parameter that “switches modes” just makes both algorithms harder to maintain. All of the commonality would be maintained in the metadata, while having the code that’s different in separate functions means that someone trying to understand it only needs to worry about one approach at a time. You leave me with the impression that the only acceptable solution to “centralization” is “one function”
I never asked you to integrate my algorithm, nor did I ever ask you to implement the metadata approach.
If I were to move forward with this, I would not want to bring in your commit. I anticipate that you would cry foul about “wasted effort” on your part if I were to do that and I’d rather not deal with that.
My impression that you are being territorial may be imagined, but listing off your accomplishments reinforces that notion.
Then I will hold on my commit and will await your code that integrates everything with your metadata approach, which will include the original code_set_search() function. If you never planned to completely include that function in your refactor, then you likely aren’t grasping the big picture here and then my code will make more sense. My posts are not meant to offend you, but are part of the code review process. Again, if you need guidance on the process of code reviews, then I suggest contacting myself and/or other contributors offline.
Can I get a confirmation that you indeed plan to fully refactor the code type module (including the original code_set_search() function) and an ETA. It looks like the mechanism to import/support other language SNOMED codes is not being actively worked on and I may have an open window of time coming up soon to work on this; trying to figure out what code to base my work off of.
define_external_table($code_external_tables,10,'sct_concepts','ConceptId','FullySpecifiedName',array("ConceptStatus=0","FullySpecifiedName LIKE '%(trastorno)'"));
Thanks for taking this on. It is your baby now. Let us know when it’s ready for testing/review.
One option you can feel free to consider is only using your sequential search mechanism in the searching algorithm. After playing around with your mechanism in all of the places that use the search feature in openemr, your searching is markedly superior everywhere including the Administration->Services searching (ie. I see no reason even for the old searching algorithm if you ask me). Here’s the commit that implements your algorithm everywhere to see what I mean (to avoid yourself getting angry, don’t look at the code, just try it out and note the function is really only called in about 5 places in the openemr codebase; note how much it even works better in Administration->Services which is where most of the function parameters come in from): http://github.com/bradymiller/openemr/commit/13a576ce3745d7a4fc287cb41ef37724376a79e4
(just food for thought)
I’ll likely just base the non-english snomed on the current code but not commit it to the codebase until your done (ie. I’ll integrate it at that point). The changes should be minimal; need to switch the table and lose the proceeding id term in the description output. This is a minor issue that can be finalized later, but instead of using separate ct_external variables for each language (which requires the user to manually change it in Administration->Lists->Code Types), planning to automatically figure this out by querying the standardized_tables_track, which stores the language SNOMED version).
Anyways, excited to seeing the completed code. And don’t take too long
-brady OpenEMR
Is there are particular deadline? Why the rush? I am donating my time for this refactor because it’s the right thing to do for the project as a whole, even though my time might be better spent pursuing “paying gigs.”
I understand your approach of deriving “sequential_code_set_search” from calls to code_set_search with mode=“code” and mode=“description” and agree that it is the correct general approach.
your searching is markedly superior…. I see no reason even for the old searching algorithm…
And here in lies the fundamental technical reason I implemented an ICD9 only version and initially sacrificed support for ICD10/SNOMED. I wish you had loaded the ICD9 codes into the “codes” table like I had suggested and tried it out. I suspect that our conversations would have been about “how do we make this work for the other codes?” instead of “why are you doing things differently?”
The changes are pretty significant, and I’d like to spend some time testing. I also have personal travel plans over the next few days. So please be patient.
uh, as described before the don’t take to long (with a sideways happy face), I am just excited to see the code. No deadline or anything like that…
Regarding the code review process, it is not servitude. Here is the initial review: http://github.com/yehster/openemr/commit/5a72d1059ddaa62d367c297f1a217f69c188982c
And my comment regarding your search from the review is below:
The nice thing about the current Code Type coding is that the user has the ultimate decisions as to which code set and where they are stored (see Lists->Code Types) and the current codebase supports this 100%. My suggestion on your search feature is to build upon it rather than sidestep it. You are not required to ensure ICD10/SNOMED/etc. all are implemented. My advice would be to:
1. Add another parameter to the end of the function code_set_search(), such as “mode” (so could have “original” and “separate_words” or somthing like that;this then opens the door to other searching algorithms also)
2. Use this parameter to place your functionality in the queries (if desired, could just work it into the codes-line272 search and ICD9-line372 search; but of course feel free to implement in other places; just comment a what modes are supported or not supported; note that not being supported isn’t a big deal because it will then simply use the “original” mode)
This way you will have added something very useful without any real chance of breaking anything (since the “original” mode is still used in the other places in the codebase that call this function) and will continue to ensure 100% of the code base supports any Code Type/setting.
In my above comment, not only did I tell you to integrate it with the other codes, but I even began to tell you how to do it. I did not need to explicitly test it (ie. I had faith in you) and by that point had already spent enough time on your review(ie. had other things to do).
Again, I am happy to to discuss the code review process offline with you anytime. And I look very forward to your highly valued improvements.
Just FYI, here’s my code for implementing Spanish (and any other language) SNOMED code sets in the main codebase. The snomed tables are very nice, because do not even need to know the language of the term key (ie. such as disorder, finding). This is just FYI, don’t implement it; still a very large amount of testing to do. My thoughts here procedure-wise are:
1. Await completion of your code set refactor (again, just pretend my spanish stuff doesn’t exist to avoid lengthening the required testing time for the patch release) and fee sheet code
2. Then when comfortable with it in the main codebase, will get it out in the next 4.1.1 patch
3. Then I’ll work on integrating the SNOMED translation/spanish stuff into your code (a potential challenge here since it’s joining two tables, but I already think I have a nice way to incorporate it into your metadata methods which will be especially nice since this join method will be re-used for every snomed term we need in the future (finding, disorder, etc.).
4. Then I will remove the count parameter. I honestly have no clue why I put that in there (I should of done the counting of the resource outside the function call in the Adminsitration->Services script) and it’s beginning to stick out like a sore thumb now that you’re removing all the bloat.
