Parsing record-jar format in PHP 5.3

Question

Here is a simple function I wrote to parse record-jar data. I need it in a php5.3 application to validate bcp47 language tags. It converts the language subtag registry among other things, into a nested php array. Any suggestions on how I can improve it?

/**
 * Quick script to parse any record-jar data into an array.
 *
 * In particular, this is of use to someone trying to validate bcp47 language codes.
 * @see http://tools.ietf.org/search/bcp47#section-3.1.1
 */
function parse_record_jar($record_jar_string) {
  $records = array();
  $record = array();
  foreach(explode('%%', $record_jar_string) as $recordstr) {
    array_splice($record, null); // truncate $record
    foreach(array_filter(explode("\n", $recordstr)) as $line) {
      $parts = array_map('trim', explode(":", $line, 2));
      if (count($parts) === 2) {
        list($key, $value) = $parts;
      } else {
        // $key retains its value from the previous iteration.
        $value = $parts[0];
      }
      if (!isset($record[$key])) {
        $record[$key] = '';
      }
      $record[$key] .= $value;
    }
    $records[] = $record;
  }
  return $records;
}

Answer 1

I have rewritten the script in another fashion. Beware all your code is almost exactly the same, but not entirely. If you are looking to just improve your script a bit, look closely at the parser.

A number of personal preferences I would like to share that I thought while reading your code but which I did not fix in my own code in all cases:

• I like (foo === false) better then (! foo) or (foo !== true).

• Strings like '%%' should be replaced with constants, preferably some of the numbers also.

• Complex logic should be split up. Even my example does fully split up the responsibilities.

• Comments should be plentiful (and meaningful!).

The code example is obese compared to the fine script you made. But I find it

• Faster to debug
• Testable
• Easy to replace dependencies
• Easy to change how the converter works (just write a couple parsers and converters)

My code example is far from done. The code should strive towards SOLID principles (which this does not). The main issue I've addressed is to clarify some of the mixed responsibilities in your code. But the new code still has some issues, mainly missing tests and SRP violations (in my code the parser is doing the converter's job). This is to show the disadvantage of bundled complex code that handles too many responsibilities.

I used 5.4 array syntax. PHP 5.3 series will enter an end of life cycle and has only received critical fixes as of March 2013.

As mentioned the code is undone (as in breaks SOLID). If you want to further improve it, you could start by moving the converting part (toArray) of the algorithm into the ToArray class.

namespace jarrecord;

Interface Parser {
    public function parse($parsable);
}

Interface Converter {
    public function convert($convertable);
}

class StringParser implements Parser {
    const RECORD_SPLIT_TOKEN = '%%';
    const KEY_VALUE_SPLIT = ':';
    /**
    * Splits and trims unparsed lines and returns array with key value pair.
    *
    * @return array
    */
    private function splitUnparsedLineToKeyAndValue($line) 
    {
        return  array_map('trim', explode(self::KEY_VALUE_SPLIT, $line, 2));
    }

    private function splitToUnparsedRecords($recordJar)
    {
        return explode(self::RECORD_SPLIT_TOKEN, $recordJar);
    }

    private function splitToUnparsedLines($unparsedRecord)
    {
        $lines =  explode(\PHP_EOL, $unparsedRecord);
        // filter to remove null lines

        return array_filter($lines);
    }

    /**
    * Takes a array of strings and returns a record-like array
    * @param string $unparsedLines
    * @return array
    */
    private function unparsedLinesToRecord($lines)
    {
        $record = [];
        foreach($lines as $line)
        {
            $partialRecord = $this->splitUnparsedLineToKeyAndValue($line);
            if(count($partialRecord) === 2)
            {
                list($key, $value) = $partialRecord; 
            }
            else
            {
                // $key retains its value from the previous iteration.
                $value = $partialRecord[0];
            }
            if(isset($record[$key]) === false)
            {
                $record[$key] = '';
            }
            $record[$key] .= $value;
        }

        return $record;
    }

    public function parse($parsable) 
    {
        $unparsedRecords = $this->splitToUnparsedRecords($parsable);
        foreach($unparsedRecords as $unparsedRecord)
        {
            $unparsedLines = $this->splitToUnparsedLines($unparsedRecord);
            $record = $this->unparsedLinesToRecord($unparsedLines);
            $records[] = $record;
        }

    return $records;
    }
}

class ToArray implements Converter {
    private $parser;

    /**
    * @param Parser $parser
    */
    public function __construct(Parser $parser)
    {
        $this->parser = $parser;
    }

    /**
    * Parses a Record-Jar string and returns it as a array.
    * @throws InvalidArgumentException
    * @param string $unparsedRecordJar
    * @return array
    */
    public function convert($unparsedRecordJar)
    {
        return $this->parser->parse($unparsedRecordJar);
    }
}

$parser = new StringParser();
$Converter = new ToArray($parser);
$array = $Converter->convert($source));

Answer 2

I'm no PHP expert, and this might be personal preference, but I like to initialize variables at the top of the function to make it clear that their scope is not limited to the constructs in which they're defined.

function parse_record_jar($record_jar_string) {
  $records = array();
  $record = array();
  $recordstr;
  $line;
  $parts;
  $key;
  $value;
  foreach(explode('%%', $record_jar_string) as $recordstr) {
    array_splice($record, null); // truncate $record
    foreach(array_filter(explode("\n", $recordstr)) as $line) {
      $parts = array_map('trim', explode(":", $line, 2));
      if (count($parts) === 2) {
        list($key, $value) = $parts;
      } else {
        // $key retains its value from the previous iteration.
        $value = $parts[0];
      }
      if (!isset($record[$key])) {
        $record[$key] = '';
      }
      $record[$key] .= $value;
    }
    $records[] = $record;
  }
  return $records;
}

Other than that I'm having a hard time finding a way to improve the code.