XML parser using PHP

Question

It is just one of the files. I have also tried to write some tests using PHPUnit. Please give me some suggestions to improve my coding-writing skills.

The below is the test file for the above file:

<?php class TestParseXML extends PHPUnit_Framework_TestCase { public function testSetup() { $objectParseXML = new ParseXML("test.txt"); return $objectParseXML; } /** * @depends testSetup */ public function testParseObject($objectParseXML) { $objectParseXML->current_line = "<Article>"; return $objectParseXML->current_line; } /** * @depends testSetup * @depends testParseObject */ public function testtagNameOn($objectParseXML, $currentLine){ $this->assertEmpty($objectParseXML->tree); $objectParseXML->scanCharacter('<', 0); $this->assertTrue($objectParseXML->isTagName); } /** * @depends testSetup * @depends testParseObject */ public function testTagContentOn($objectParseXML){ $objectParseXML->isTagName = True; $objectParseXML->scanCharacter('>', 3); $this->assertTrue($objectParseXML->isTagContent); $this->assertNotTrue($objectParseXML->isTagName); } } ?> Code Files: <?php require('XML.php'); class OpenXML extends XML { var $fileHandler; public function openXMLFile($filename, $mode='r'){ $this->fileHandler = @fopen($filename, $mode); } public function getHandler() { return $this->fileHandler; } public function getHandlerType(){ return get_resource_type($this->fileHandler); } } ?> <?php require('ReadXML.php'); class ParseXML extends ReadXML { public $handle; public $current_line; public $isTagName = False; public $isTagContent = False; public $startParse = False; public $tagName; public $tagContent; public $tree = array(); //Call appropriate function depend on $isTagName and $isTagContent public function scanCharacter($char, $index) { if(strcmp($char, '<')==0) { if($this->isTagContent){ $this->isTagContent = False; } if($index+1 < $this->getLen()) { $this->isTagName = $this->tagNameOn($this->current_line[$index+1]); } } elseif($this->isTagName and (strcmp($char, '>')==0)) { $this->isTagName = $this->tagNameOff(); $this->isTagContent = $this->tagContentOn(); } elseif((strcmp($char, '/')==0)){ $this->isTagName = False; $this->isTagContent = False; } elseif($this->isTagName and !strcmp($char, ' ')) { $this->isTagName = $this->tagNameOff(); } elseif($this->isTagName) { $this->gatherTagName($char); } elseif($this->startParse and $this->isTagContent) { $this->gatherTagContent($char); } } public function tagNameOn($nextChar) { if(!strcmp($nextChar, '/')==0) { $this->tagName = ""; return True; } // print_r($this->tagName); // print_r("\n"); // print_r($this->tagContent); $this->isTagContent = False; return False; } public function tagNameOff() { if(!($this->startParse) and strcmp($this->tagName, 'Document') == 0){ $this->startParse = True; } return False; } public function getLen(){ return strlen($this->current_line); } public function tagContentOn() { return True; } public function gatherTagName($char){ $this->tagName.= $char; } public function gatherTagContent($char){ $this->tagContent.= $char; } public function getTag() { $this->tagContent = trim($this->tagContent); if($this->tagName and $this->tagContent){ if(!$this->isTagContent){ array_push($this->tree, array($this->tagName, $this->tagContent)); $this->tagContent = ""; $this->tagName = ""; } } } public function getTagContent(){ $listNames = array(); forEach($this->tree as $value){ array_push($listNames, $value[1]); } return $listNames; } public function getTagNames(){ $listNames = array(); forEach($this->tree as $value){ array_push($listNames, $value[0]); } return $listNames; } public function readLine() { $this->handle = $this->getHandle(); $this->current_line = fgets($this->handle); while($this->current_line) { for ($i = 0; $i < $this->getLen(); $i++) { $char = $this->current_line[$i]; $this->scanCharacter($char, $i); } $this->getTag(); } return $this->tree; } public function goOverLine(){ } } $a = new ParseXML('../IMQ+AZIBPrototyp.xml'); $a->openXMLFile($a->getFileName()); $a->acquireHandler(); $a->readLine(); print_r($a->getTagContent()); ?> ReadXML.php <?php require('OpenXML.php'); class ReadXML extends OpenXML { var $content; var $line; public function acquireHandler() { $this->content = $this->getHandler(); } public function getHandle() { return $this->content; } public function isReadLine() { return feof($this->content); } public function getLine() { if(!($this->isReadLine())) { $this->line = fgets($this->content); return $this->line; } } } ?> 

Answer 1

Good to see you are using PHPUnit as testing method :)

Issuelist:

• Curly brackets are sometimes in same line as a method header and sometimes in the following
• There are both empty methods and methods that return always true/false
• Not every method has a self-describing name
• There are global attributes with the access modifier public
• There are multiple returns in one method
• Classes are included manually
• PHP-tags are closed

Recommendation

Curly brackets are sometimes in same line as a method header and sometimes in the following

For the sake of a good code-reading and understanding code should be structured. Write curly brackets either in the same line as the method header or in the following.

There are both empty methods and methods that return always true

ParseXML::goOverLine(): When a method body is empty it usually means you have not implemented its logic yet. It is possible that one forget to include its logic but calls the method which can leads to a difficult to identify bug at a later time. Therefor I recommend to throw an Exception with message Method not implemented.

ParseXML::tagContentOn(), ParseXML::tagNameOff(): Why does this method returns true/false anytime? Does it switches tags on/off?

ParseXML::gatherTagName($char), ParseXML::gatherTagContent($char): These methods rather append, do they? Or does this word describes appending as well?

Not every method has a self-describing name

ParseXML::getLen(): What does it return? Object Length, Current Line Length, ...? I recommend to rename the method to what it does - getCurrentLineLength().

There are global attributes with the access modifier public

An object is responsible for its valditity. As of that its attributes has to be setted via setters always and the attributes has to have as access modifier either protected or private.

There are multiple returns in one method

This makes the code less maintainable. Having more than one return means there are multiple scenarios when the method can be stopped. In case of a bug one need to debug through the whole method to figure out the return-point.

Classes are included manually

Instead of including classes manually it is recommended to make usage of the autoloader function (http://php.net/manual/en/function.spl-autoload-register.php). The advantage is that you don't have to worry about including a class.

PHP-tags are closed

It is not recommend to close the PHP-tag. It can happen that you have an empty space after the closed PHP-tag which leads to headers already sent error. Not closing them reduces headaches :)

Answer 2

The explicit @depends sound like you don't need them. A cursory look at the documentation suggests that the setup method should be either called setUp, or be annotated with @before if you really don't like the name.

Test functions also don't need to return anything.

Apart from that looks good except for the occasional indentation and whitespace issue; that might be due to pasting, but in general it's better if the code looks first of all consistent and secondly adheres to some sort of style guide.

Answer 3

Unfortunately the code you posted here isn't complete but I can notice I couple things you should work on anyway.

1. Code standards
   • Consider using an automatic code formatter and/or a code sniffer tool so that you can pick up a standard, say PSR-2, and go for it
2. Type hinting
   • I appreciate PHP has always been a weak-typed language but this has rarely been a good thing. You should be able to take advantage of type hinting for at least objects and arrays unless you're using PHP7. In that case you can type hint also scalar variables.
3. Mock the filesystem
   • In your unit-test you are storing a real file in the filesystem. You should mock the filesystem with a proper 3rd party library or just use PHP streams. In both cases you won't have to change your code but just your unit-test. As a side note, creating real files in a unit-test isn't good practice. I can think of a couple reasons:
     1. Your test isn't isolated since you're testing against a real filesystem that may fail (e.g. file permissions)
     2. If your test fails you'll have to delete the testing file manually each time. This is far from ideal. With streams your file lives in memory and it therefore gets deleted once the stream is closed.
     3. With a mocked filesystem is easier to test how your class behaves if something goes wrong (e.g. file doesn't exist, file is not readable and so on)
4. Unit-tests should be thorough
   • I can see you're testing your classes against expected inputs. But what about unexpected inputs? That's when you actually have a problem and find out that your class doesn't have a sound error management. So yes, tests should usually be thorough. Make sure you test against both bad and good inputs. PHPUnit data providers come to aid here.
5. testSetup() instead of the PHPUnit setUp() official function
   • no need for @depends here. Just create an $objectParseXML private attribute in your test case and then do your magic in the setUp() method. Check the PHPUnit documentation for that.
6. Try to avoid public attributes
   • This is just what encapsulation is for, just make your code more robust.
7. Class names
   • You're using very generic class names. Try to hide your class implementation and guess what's inside. Now ask a colleague to do the same. Probably best if you give your classes more meaningful names, right?
8. Missing PHPDoc
   • There are several reasons why it's always wise to complete your code with PHPDoc. My favourites are: better code readability, code completion and code analysis.

There are probably other things I may notice if I were to spend more time on this but I think this should be enough for a starter :-)

Also, I didn't check if there are logic issues in your code. Once you write a thorough unit-test you should be able to fix them.

Happy coding!