4
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Could you supply a snippet of the source feed to the function. The code has a high cyclomatic complexity, which you might consider splitting up. Tests would be a good idea to ensure everything works \$\endgroup\$ – Ronni Skansing Feb 19 '14 at 20:24
  • \$\begingroup\$ @RonniSkansing updated with link \$\endgroup\$ – kojiro Feb 19 '14 at 20:51
6
\$\begingroup\$

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));
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I should have put this in my edit: beware that beware does not mean what you think it means (it means be afraid, as opposed to be aware). \$\endgroup\$ – Big McLargeHuge Feb 20 '14 at 16:04
  • \$\begingroup\$ @davidkennedy85 thanks, I will try to remember that \$\endgroup\$ – Ronni Skansing Feb 20 '14 at 16:11
3
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ In dynamic languages such as Java and PHP it is often prefered to keep the variables alive for the shortest time possible and push them as far into the scope as possible. Some would argue for performence (Only in extreme cases). I think this answer hits it pretty well stackoverflow.com/a/10205934/924016 but in the end it is a preference. \$\endgroup\$ – Ronni Skansing Feb 20 '14 at 2:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.