1
\$\begingroup\$

I have to do a program in PHP that reads IGC files like this IGC file and gets records about the glider.

For now I came up with something like this:

<?php

        class Pilot {
            public $name;
            public $gliderType;
            public $competitionId;
            public $gpsDatuml;
            public $competitionClass;
            public $startPoint;
            public $endPoint;
        }
        $pilot = new Pilot();
        $myFile = new SplFileObject("https://xcportal.pl/sites/default/files/tracks/2020-06-09/069daro396091568.igc", 'r', 10);
        for($i=0; $i < 10; $i++) {
            $information =  $myFile->fgets();
            if(($pos = strpos($information, "PILOT")) !== FALSE){
                $pilot->name = substr($information, $pos+7);
            }
            if(($pos = strpos($information, "GLIDERTYPE")) !== FALSE){
                $pilot->gliderType = substr($information, $pos+12);
            }
            if(($pos = strpos($information, "COMPETITIONID")) !== FALSE){
                $pilot->competitionId = substr($information, $pos+15);
            }
            if(($pos = strpos($information, "GPSDATUM")) !== FALSE){
                $pilot->gpsDatuml = substr($information, $pos+10);
            }
            if(($pos = strpos($information, "CLASS")) !== FALSE){
                $pilot->competitionClass = substr($information, $pos+7);
            }
            // echo strpbrk($information, "PILOT");
            // echo substr($information, 10);
        }
        echo $pilot->name;
        echo $pilot->gliderType;
        echo $pilot->competitionId;
        echo $pilot->gpsDatuml;
        echo $pilot->competitionClass;
    
        $myFile = null;
    ?>

but I wonder if there is anyway to do it better that I can't get up now.

Also is it good practice to keep properties of the class as a private field or can I leave them as public if any other class doesn't use that property?

\$\endgroup\$
0
\$\begingroup\$

...but I wonder if there is anyway to do it better that I can't get up now.

Simpler loop iteration

PHP's foreach and range() could be used to replace to the for loop:

foreach(range(0, 9) as $i) {

That way the code doesn't manually have to increment $i.

Looping over properties

Initially I was thinking you could use a ReflectionClass::getProperties() to enumerate the properties for iterating over, and capitalizing the names to pass to strpos(), but some properties don't match up evenly - e.g. name, gpsDatum. A mapping array could be declared of property names to strings to search for, or else use regular expressions.

Reading file input

Did you consider using file_get_contents()? It may require using more memory since it would read the entire contents of the file into memory, but would mean the call to fgets() could be removed from the loop.


Also is it good practice to keep properties of the class as a private field or can I leave them as public if any other class doesn't use that property?

This is a subjective question. Some argue that "you always strive to hide as much as possible from either client code, but also from subclasses." but you may disagree with that so it is up to you.

You could make public methods on the Pilot class - e.g. one to do the parsing, and one to get the properties - e.g. all properties in an array, or one getter method for each property.

\$\endgroup\$
1
  • \$\begingroup\$ Can you explain which parts you disagree with? or is it the entire review? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Apr 15 at 21:28
0
\$\begingroup\$

I don't think I understand the point of declaring a class if you aren't going to write any methods in it. I mean why go the expense of declaring a class just to store some variables outside of the global scope. Assuming this class can never populate its properties without parsing a document, so it make sense to have its constructor initiate the file reading and generate values for the properties.

  • I haven't personally used SPLFileObject before, but the manual says that:

    • the 2nd parameter $open_mode defaults to r and
    • the 3rd parameter $use_include_path expects a boolean value. I expect that that 10 is not doing what you intend.
  • Reading a limited number of lines from the file with fgets() is a professional decision that shows real care and deliberate scripting. Removing all debate regarding micro-optimization or if reading the whole file is an "affordable" cost, I love that you are endeavoring to ONLY "read what you need".

  • By my count, there are 3 factors which should halt the loop of reading of lines: 1. there are no more lines in the file 2. a line is encountered that does not contain a colon 3. you have extracted all relevant details from the file up to your self-imposed "magic counter" 10 The first two logical break points should be baked into the loop's breaking syntax. The third is probably something that you don't want to get into the habit of. Another developer may look at the code (without seeing a sample input file) and think: "Why is this operation limited to 10? Why not 20? Why not 5?" It is best to avoid magic numbers, or if you need to have them, write a comment explaining why it is implemented.

  • That battery of if conditions is considerably less wise. Think about it. If the first if is satisfied -- why would you want to bother executing the other four checks? They should never be satisfied if the first one is. While I am going to suggest a different technique in my snippet to follow, your snippet at the very least would be improved by using subsequent elseif conditions.

Because I'm rather familiar with regex, I find using regex with a lookup array to be a very convenient technique to extract your targeted values from the remote file. Not only does regex afford a more compact script, it maintains the flexibility of your original script by allowing the targeted lines to exist in any order.

A code suggestion:

class Pilot
{
    public $name;
    public $gliderType;
    public $competitionId;
    public $gpsDatuml;
    public $competitionClass;
    public $startPoint;  // I don't know where/how this gets populated
    public $endPoint;  // I don't know where/how this gets populated

    private $keywords = [
        'PILOT' => 'name',
        'GLIDERTYPE' => 'gliderType',
        'COMPETITIONID' => 'competitionId',
        'GPSDATUM' => 'gpsDatuml',
        'CLASS' => 'competitionClass',
    ];
    
    public function __construct(string $url)
    {
        $this->populatePropertiesFromUrl($url);        
    }
    
    public function populatePropertiesFromUrl(string $url): void
    {
        $pattern = '~(' . implode('|', array_keys($this->keywords)) . '): (.*)~';

        $file = new SplFileObject($url);
        while (!$file->eof() || strpos($line = $file->fgets(), ':') === false) {
            if (preg_match($pattern, $line, $match)) {
                $this->{$this->keywords[$match[1]]} = $match[2];
            }
        }
    }

$pilot = new Pilot('https://xcportal.pl/sites/default/files/tracks/2020-06-09/069daro396091568.igc');
var_export(get_object_vars($pilot));

Output (should be):

array (
  'name' => 'DARIUSZ KISZKO',
  'gliderType' => 'ADVANCE IOTA 2',
  'competitionId' => '0000',
  'gpsDatuml' => 'WGS-84',
  'competitionClass' => 'Paraglider (Standard)',
  'startPoint' => NULL,
  'endPoint' => NULL,
)

*Disclaimer: I am making some assumptions about the text in the remote file. I don't actually know how the text may vary. I don't know if my suggested code will hold up against other files. You may wish to adjust my loop breaking algorithm.

\$\endgroup\$

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.