2
\$\begingroup\$

I'm parsing a raw text file with my parser and a defined template (basically an array) to create a formatted array out of it.

The idea is that each line can have different templates and can be identified with the first characters of the lines. A template means that a line has different values delimited by different lengths/positions.

I have a class, PostionalEdiParser, with 3 functions:

The parsing functions:

public function parse(array $template = [], array $file = [], $identifierSize=5)
{
    if (empty($template)) {
        $this->errors["template"] = "No template given";
        return $this->errors;
    }
    // Init array
    $my_array = [];
    // Init counter
    $i = 0;
    // SubBody counter
    $j = 0;
    $previousValue = null;
    // Count all lines
    $len = count($file);
    foreach($file as $row) {
        if ($i == 0) { // First Line (header)
            $my_array[] = $this->formatLine($template["header"], $row);
            $i++;
            continue;
        } else if ($i == $len-1) { // Last Line (footer)
            $my_array[] = $this->formatLine($template["footer"], $row);
            $i++;
            continue;
        }
        if(is_array($template["body"])) {
            foreach ($template["body"] as $index => $templateSubBody) {
                if (substr($row, 0, $identifierSize)==$index) {
                    $j++;
                    $my_array[1][$j][]= $this->formatLine($templateSubBody,$row);
                    continue;
                }
                if(is_array($templateSubBody)) {
                    foreach ($templateSubBody as $index2 => $templateSubBody2) {
                        if(substr($row, 0, $identifierSize)==$index2) {
                            $my_array[1][$j][] = $this->formatLine($templateSubBody2,$row);
                        }
                    }
                }
            }
            $i++;
            continue;
        }
        $i++;
    }
    return $my_array;

}

Then the formatLine() function that apply the specific template of the specific line:

public function formatLine($template, $data, $position=0) {
    $my_data=[];
    foreach($template as $index => $length) {
        // add current field to array with the named value
        $my_data[$index] = substr($data, $position, $length);
        // move the 'pointer' to the start of the next field
        $position += $length;
    }
    return $my_data;
}

And a getter for the errors:

public function errors()
{
    return $this->errors;
}

An example of code to understand how it works. This is a defined template and for each key, the value is the length of the fields:

$template =  [
        "header" => [
            "IDENTIFIER"=>2,
            "DOT"=>1,
            "NUMBER"=>2,
            "TEST"=>1,
            "HEADER"=>6,
        ],
        "body" => [
            "41.00" => [
                "IDENTIFIER"=>2,
                "DOT"=>1,
                "NUMBER"=>2,
                "TEST"=>1,
                "CONTENT"=>8,
            ],
            "lines" => [
                "41.20" => [
                    "IDENTIFIER"=>2,
                    "DOT"=>1,
                    "NUMBER"=>2,
                    "TEST"=>1,
                    "CONTENT"=>8,
                    "SPACE"=>1,
                    "SUBLINE"=>8
                ]
            ]
        ],
        "footer" => [
            "IDENTIFIER"=>2,
            "DOT"=>1,
            "NUMBER"=>2,
            "TEST"=>1,
            "CONTENT"=>6,
        ]
    ];

And this is a file parsed to a rows/lines array:

$rows = [
        "00.00 HEADER",
        "41.00 CONTENT1",
        "41.20 CONTENT2 subline1",
        "41.20 CONTENT2 subline2",
        "41.00 CONTENT3",
        "99.00 FOOTER"
    ];

By calling this function $resultEdi = $parser->parse($template, $rows);, I'll get the following result:

array:3 [▼
  0 => array:5 [▼ // Header
    "IDENTIFIER" => "00"
    "DOT" => "."
    "NUMBER" => "00"
    "TEST" => " "
    "HEADER" => "HEADER"
  ]
  1 => array:2 [▼ // Body
    1 => array:2 [▼ // Group 1
      0 => array:5 [▼ // Group 1 header
        "IDENTIFIER" => "41"
        "DOT" => "."
        "NUMBER" => "00"
        "TEST" => " "
        "CONTENT" => "CONTENT1"
      ]
      1 => array:7 [▼ // Group 1 line
        "IDENTIFIER" => "41"
        "DOT" => "."
        "NUMBER" => "20"
        "TEST" => " "
        "CONTENT" => "CONTENT2"
        "SPACE" => " "
        "SUBLINE" => "subline"
      ]
    ]
    2 => array:1 [▼ // Group 2
      0 => array:5 [▼ // Group 2 header
        "IDENTIFIER" => "41"
        "DOT" => "."
        "NUMBER" => "00"
        "TEST" => " "
        "CONTENT" => "CONTENT3"
      ]
    ]
  ]
  2 => array:5 [▼ // Footer
    "IDENTIFIER" => "99"
    "DOT" => "."
    "NUMBER" => "00"
    "TEST" => " "
    "CONTENT" => "FOOTER"
  ]
]

Since it's my first time creating a parser, I have different question for this code:

  1. As I'm trying to make it easy to customize, would it be better to make the header/footer processing optional?
  2. What is a proper way to do error handling?
  3. There is a "sub template" (here it's the lines template) with a 2 dimensional array. Would it be smart to create a recursive function outside of the parse(), so that we can have deeper templating possibility?
\$\endgroup\$
1
\$\begingroup\$

Your template seems odd.

If your input is a text file that parses into rows like:

$rows = [
    "00.00 HEADER",
    "41.00 CONTENT1",
    "41.20 CONTENT2 subline1",
    "41.20 CONTENT2 subline2",
    "41.00 CONTENT3",
    "99.00 FOOTER"
];

Then it would seem the ##.## number is behaving somewhat like a key to determine the parsing template. So shouldn't your "template" use these key for lookup like:

$template = [
    "00.00" => [ // parsing template for header type ],
    "41.00" => [ // parsing template for body type ],
    "41.20" => [ // parsing template for line type ],
    "99.00" => [ // parsing template for footer type ]
];

Or something of this sort, such that you don't have to traverse this multi-level template structure. You just seem to be wasting a lot of iteration looping over the template because it may not be structured in an optimized fashion.


If header and footer are always going to be first and last line as might be indicated from your example, should these even be in the same template structure? You could just as easily have $headerTemplate and $footerTemplate variables defined for those use cases.

You could also address the header and footer case outside the loop to avoid the unnecessary if nesting in loop.

i.e.

$my_array[] = $this->formatLine($headerTemplate, $rows[0]);
for ($i = 1; $i < count($rows) - 1; $i++) {
    // run parser on body lines
}
$my_array[] = $this->formatLine($footerTemplate, $rows[count($rows) -1]);

This also cleans up all your different $i++ statements in this loop.


    if(is_array($template["body"])) {
        foreach ($template["body"] as $index => $templateSubBody) {

Why would you even even try to process the file if $template['body'] is not properly defined? Why must this condition be checked on every iteration through the loop. It seems this conditional should be designed out of this location.

You have similar possibly unnecessary conditional around $templateSubBody


if (empty($template)) {
    $this->errors["template"] = "No template given";
    return $this->errors;
}

Is this strong enough validation for the template (see comment above)? Should you throw an exception here, or do you really expect the caller to pass an empty variable here in course of normal code operations?

Why are you returning $this->errors directly here if you have a getter for this?

You are also, placing the burden on the caller to decipher the difference between an error array returned result vs. a normal parse result. Why place this burden on the caller. If you don;t want to throw an exception, perhaps the following would be better

$this->errors['template'] = ...;
return false;

That way if the caller wants to get the error message later they can (via your getter), but it is really easy for them to differentiate a good result from a bad result on the method call.


Since this is part of a parsing class, should you perhaps set the templates on the class (with appropriate validation in doing so) before going about parsing input? Should you pass template(s) to constructor? (Just thinking out loud here in that I don't know the call pattern you are imagining in your application against this class.) If your call pattern is such that you will be parsing multiple files against multiple templates within any single script execution, then I wonder whether this parser makes sense to be a concrete class at all, since for this use case you would not ever seem to be maintaining state in this class (you are always passing your dependencies to this parse method, meaning it might as well be static).

The only "state" this class has is with regards to the error property, which is pretty much used in a stateless manner. You directly return the value from method immediately after setting this state in the only place in the class this property is set. Why do you need $this->errors and corresponding setter at all if this is the only way it is being used?


What is significance of DOT and TEST portions of template? These just seem like separators between actual fields of importance. What value does it have to put DOT and TEST in the resulting data structure?


The third parameter to formatLine() does not seem to ever be used (other than for it's default value). Is it needed? This method is also stateless and perhaps should be static.


Building on the above, what value is there in even parsing a known identifier?

"41.20" => [
                "IDENTIFIER"=>2,
                "DOT"=>1,
                "NUMBER"=>2,

You know that this template will always result in:

                "IDENTIFIER" => "41",
                "DOT" => ".",
                "NUMBER" => "20",

So why parse it? Why is the resulting data structure not just something like the following?

          "41.20" => ["CONTENT" => '...', "SUBLINE" => '...']

// SubBody counter
$j = 0;

You should probably define this counter directly before the loop in which it is used. Also, I am not quite sure why you are using counters here in this manner as opposed to just using standard for loop or foreach (.. as $i => $value) is you really want to use foreach. You are adding a lot of clutter in your code that does not need to be there.

I think this is perhaps because you are building an odd resulting data structure which seems to rely on numerically-index values when perhaps you should use associative keys. Why 0,1,2 for key values in result array versus more meaningful header, body, footer (which actually matches your template structure better)? Why does your "body" element have it's first numerically-indexed value starting at 1 instead of zero (which would be unexpected for a numerically indexed array in PHP)?

If I was writing a piece of code working with this class, the resulting data structure would be very hard for me to understand. This should be readily apparent by the fact that you felt it necessary to comment your own data structure to help explain what it means.

Before thinking about making the system more flexible, you should make sure it is understandable in template definition, in usage, and in it's results. Making it understandable to humans will probably do wonders in helping you guide your design to something that is more flexible (as is often the case when you try to think of your code in "real world" terms).


You should strongly consider biasing yourself towards using exact comparison operators vs. loose comparisons as you are using throughout. The loos comparisons tend to surface unexpected bugs/behavior around truthy/falsey conditions. You should perhaps only use them when there is specific reason to do so.


Stylistically,

  • You should not mix snake_case and camelCase in your own code.
  • Your spacing around flow control operators, comparison operators, assignment operators, etc. is inconsistent, making your code harder to read. You should strive to develop a consistent style (ideally using PSR-1 and PSR-2 as strong influences on that style).
  • You are inconsistent in using lowercase vs. uppercase in your associative array keys. Perhaps this is design requirement, but looks odd if not.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for the time you took to answer. You’re right that I want to parse different inputs with different templates, so it means I shouldn’t create an object and create static method ? Or I should create an object with templates given at constructors for each ? The third parameter of formatLine is there to be able to apply an offset if needed but you’re right, maybe it should be static. I wanted to keep the structure the same as the template, to avoid losing data. Maybe it’s totally useless. DOT and TEST were here for example, it’s not a real template. \$\endgroup\$ – davidbonachera Jun 6 '17 at 3:42
  • \$\begingroup\$ Also with your point of view on the template, how can I create a "group" with all the sublines ? Here with this template, I can understand that 41.20 is inside 41.00. \$\endgroup\$ – davidbonachera Jun 6 '17 at 7:06
  • \$\begingroup\$ Also @mike-brant I can't have the index of my array as an identifier because sometimes I may have two times the same identifier like two 41.20 following each other. \$\endgroup\$ – davidbonachera Jun 6 '17 at 9:09
  • \$\begingroup\$ @davidbonachera The template approach I suggest could accept any number of 41.20 lines in any order. You are in essence saying that any line that beings with 41.20 is treated as "subline" and should be parsed according to the instructions associated with 41.20. With regards to concrete vs. static usage of class/methods - typically you only need to instantiate an object if there is state to be stored. I don't really see how this is applicable here if you are not storing templates on the object. \$\endgroup\$ – Mike Brant Jun 6 '17 at 15:16

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.