I'm new to PHP, having coded in Perl and ColdFusion years ago. In the process of trying to familiarize myself with my new job, I have found some code that would neither seem efficient nor logical, although it does execute properly.
I would like to get feedback about whether one particular piece of code is poorly written, or whether there is in fact a reason it was done this way that is not apparent to someone new to PHP.
The code reads in the contents of a text file, and then sets variables from that file. The input file contains customer records, with fields/columns separated by commas and rows separated by line breaks. After the variables are set, they are sliced and diced in various ways, and at the end of each foreach block, a row is appended to another text file.
The data in the input text file might look something like this:
123456789,MOSName,Bob,Jones,2012 Parakeet Lane
999999999,OtherMOSName,Samantha,Smith,1212 Whatever Street
236751235,YetAnotherMOSName,Tom,Baker,555 Blahblah Boulevard
... and so on.
The below code only includes what is essential to understand the question.
$openit = file('thedirectory/thefilename.txt');
foreach($openit as $values) {
//acct[0]
$acct = explode(',', $values, 2);
//mos[1]
$mos = explode(',', $values, 3);
//f_name[2]
$f_name = explode(',', $values, 4);
//l_name[3]
$l_name = explode(',', $values, 5);
//addr[4]
$addr = explode(',', $values, 6);
...and so on, for 26 variables.
My inclination would be to simply explode the string once for each row, and place each value in a variable, like:
list($acct, $mos, $f_name, $l_name, $addr) = explode(",", $values);
But instead each variable is being set as an array of N items, where N is the limit attribute of explode, and where the final item includes the entire rest of the string. Then the actual piece of data is being referenced in the rest of the file using the element of that array where the relevant info is actually stored. So, acct would be referenced later on in the code as $acct[0], mos would be referenced as $mos[1], etc.
Is this as inefficient and illogical as it seems? Or what am I missing here? If it is indeed an odd way of doing things, what would have been the motivation to do it this way?
1 Answer 1
Is this as inefficient and illogical as it seems?
Yes- as others have pointed out in comments, it seems to be somewhat poorly written. Not only is it difficult to read (as evidenced by your question here), but it isn't as efficient as it could be- each variable (e.g. $acct, $mos) is an array -and each sequential array grows in size! This is quite wasteful - see this playground example for an illustration.
If there are 26 fields per row, that means 376 elements* across 26 arrays. Storing each value as a string (or cast numbers to integer/float values) would dramatically decrease the amount of memory used. While the memory would likely be reclaimed between iterations, it would still make quite a difference.
If it is indeed an odd way of doing things, what would have been the motivation to do it this way?
That would be difficult for us to answer without knowing the history of the team, code, etc. Perhaps the original implementor(s) didn't know any other technique for abstracting those values.
My inclination would be to simply explode the string once for each row, and place each value in a variable, {and use
list()}
That sounds like a better choice, though if there truly are 26+ values in each row, that line of code would get very lengthy. Perhaps a good choice would be to define a mapping of indexes like below, or just assert (/use error handling) to ensure the expected data exists in each line. Or, as Claudio mentioned in a comment str_getcsv() could also be utilized to simplify things here.
const FIELD_MAPPING = array(
'acct',
'mos',
'f_name',
'l_name',
...
);
It is unclear how the variables are used but presumably they are passed as parameters to a function/method call. If that is the case, then call_user_func_array() could be used, or if PHP 5.6+ is used, then call the function directly and pass the parameters using the spread/splat operator.
*Triangle series sum of 26 (351) + 26 - 1 (since each array except the last one has an extra element (the remaining fields separated by commas)
$csv = array_map('str_getcsv', file('file.txt'))and then process it? \$\endgroup\$