I thought I’d try tinkering with my first PHP program. I seem to pick up languages more easily by just diving into existing code, rather than starting with a reference manual or tutorial. Since list_staged.php has been exhibiting errors for us, I chose it as my victim for a cleanup.
I see where some reorganization can make the module more self-documenting and considerably reduce the depth of unnecessarily nested if statements. Statements like “if (!strpos($file, “.zip”) !== false)” would seem clearer written as “if (substr($file, -4) !== ‘.zip’)”. I see a lot of little tweaks to make the code more logical, concise and readable.
One general thing that is bugging me is…
When PHP is echo()'ing text into the HTML, it rarely does so directly. Instead it echo()'s the result of sending the output string through functions such as text(), attr() and xlt(). The text(), attr(), and xlt() functions are not found in the PHP reference manual, so I *ass*ume they have to be user-defined functions. Yet, a case-insensitive scan of the entire xampp folder (including subdirectories) finds no files containing the string “function attr”.
Where do I find the source for these text-formatting functions, so that might understand their purpose?
Thank you.
Would incorrectly flag a file if the “.zip” was not at the very end of the file name.
Everyone’s got coding style quirks/preferences, but if you find things that are unclear, comments that clarify what is going on would be better than making changes unless you have a good plan to test things. The depth of some statements you are looking at may in fact be addressing things the original developer realized but you don’t.
Unless it is trivial, "Self documenting"code is a myth.
I see… they are wrappers for the htmlspecialchars() built-in function.
Thank you.
I would think it would be correct to flag (discard) anything unless the extension were .zip.
If accepting an embedded “.zip” anywhere within the filename is desired, then sticking with the strpos() makes sense, although the boolean Not along with a Not operator are superfluous and make the comparison unnecessarily complex.
The depth of some statements you are looking at may in fact be addressing things the original developer realized but you don’t.
That is possible, or it may be things that a fresh pair of eyes will recognize as a candidate for clean-up.
For instance, I don’t see the point in nesting half of the entire function inside of this test, resuling in a huge nested statement going 5 levels deep and forcing you to go searching to the end of the function to see what takes place when the test fails:
if (is_dir($mainPATH)) {
// a hundred lines of code in here with 4 more levels of nesting
}
else {
?><div class="error_msg"><?php echo xlt("The installation directory needs to be created.");
?><span class="msg" id="<?php echo attr($db); ?>_dirmsg">!</span></div><?php
}
When you could perform this test in this location and reduce the size/complexity of the massive over-nested statement that follows.
$mainPATH = $GLOBALS['fileroot']."/contrib/".strtolower($db);
if (!is_dir($mainPATH)) {
?><div class="error_msg"><?php echo xlt("The installation directory needs to be created.");
?><span class="msg" id="<?php echo attr($db); ?>_dirmsg">!</span></div><?php
exit;
}
"Self documenting"code is a myth.
I’ve been writing it for 30 years.
I find meaningless verbage injected as ‘documentation’ where it is not required as much of a hinderance to readability as neglecting to comment on something complex that actually merits an explanation.
I do realize that strpos() can return either a positive integer, or a boolean False. Using “!” to force an integer return to be a boolean True prior to the compare, in testing, shows to be unnecessary. It’s probably a case of 6-of-one and a half-dozen of the other as to whether using strpos() or substr() is more appropriate, as I doubt NLM will begin releasing RxNorm or SNOmed files with names such as Snomed.zip.xxx
Got it, I realize now that you are proposing switching from strpos to “substr ==”. My bad. Yup, the change you want to makes sense.
This is why git is a much better method for discussing code changes themselves rather than the sourceforge forums,
so I suggest that if you want to take on the task of cleaning things up that you make sure to use git to track your changes.
I find meaningless verbage injected as ‘documentation’ where it is not required as much of a hinderance to readability as neglecting to comment on something complex that actually merits an explanation.
A true statement. I suspect that at the core our development philosophies are similar.
. I see a lot of little tweaks to make the code more logical, concise and readable.
Go for it and submit your changes for review on github.
That is possible, or it may be things that a fresh pair of eyes will recognize as a candidate for clean-up.
There is lots of spaghetti code in OpenEMR and if you are willing to devote the resources to code fixes and testing it will be beneficial to the community.
Oh, but don’t forget to make your substr implementation case insensitive. .ZIP, .Zip and .zip should all be acceptable. Filename issues with windows vs. unix… uggh
Oh dear… git-what?
github?
More new stuff to absorb!
(My brain is going to explode)
It’s some sort of discussion/review/tracking/versioning system?
I’ll have to look around for it…
I’m sure there will be a period of digesting to do before feeling confident enough to properly participate in or contibute to github.
Paul,
I had recently added the ICD 9/10 data load code to openEMR with input and support from the community that you’re looking into. I agree, the xlt() stuff was something I left to the end. Be aware that I modified the original code that previously supported the RXNORM and SNOMED stuff and changed how the files were managed so we could load both ICD9 and ICD10. Like you, it was my first attempt at PHP, so I’m sure you’ll find a number of “cleanup items”. It’s been a few months but I’ll be tracking this thread and see if I can add any tidbits along the way.
Paul,
Another comment related to your other “RxNorm and SNOMED installs unresponsive.” thread is that I only tested the UNIX / MAC OS X platforms. When the code was originally posted to the hub, I know there were some other community members who did have success with the feature on the Windows platfomr. So you may encountered some PHP/WIndows issues that I hadn’t encountered in any of this other testing. I think your comments on the other thread about PHP / OS versions and might be coming into play on some of those UI issues you’ve encountered.
