Finding the right structure for my application

Question

I have three different types of XML files (structure) which I would like to import into a database.

Every hour a cronjob calls my import.php with a specific XML file type as a parameter.

The import script includes a bootstrap.php (AutoLoader, Setup Doctrine DB Connection) and puts all files matching the name convention of this type in an array and calls a static method for this class.

Simplified code (import.php):

<?php
require_once("bootstrap.php");
function getFiles($pattern)
{
 $fileList = NULL;
 foreach( glob('xml/*'.$pattern.'.xml') as $filename )
 {
 $fileList[] = $filename;
 }
 return $fileList;
}
switch($xmlType)
{
 case "TypeA":
 TypeA::importFeed(getFiles($xmlType));
 break;
case "TypeB":
 TypeB::importFeed(getFiles($xmlType));
 break;
case "TypeC":
 TypeC::importFeed(getFiles($xmlType));
 break;
}

Currently, the classes (TypeA, TypeB, TypeC) contain no other method except importFeed.

Inside importFeed, only 2 things are equal in each class:

1. Getting the entityManager (Doctrine) by calling $em = app::getEm();
2. Convert XML into SimpleXMLElement using my parser class $feedObj = Parser::xmlToObject($xmlFile);

Everything else in importFeed differs from class to class because of the different XML types. Only a few tasks repeating e.g. check if an entity already exists in the database.

My first thought was creating a Helper class for this and some other repeating tasks I also need outside the import-script in this application.

But I'm not sure if this helper class should be a normal, "static" or "singleton" class.

The helper-methods doesn't need a specific object instance so static-methods would a good choice. But in some methods I need access to the DB and I do not wont to call app::getEm(); in every method because I don't think this is a good/clean solution.

At this point I challenged my current application design and read a lot of articles about this. The point is the application is not large and it should not be so using a Framework like Symfony is completely exaggerated - nonetheless I want a good solution.

All three classes (TypeA/B/C) have the same function that looks like this:

public static function importFeed($xmlFiles)
{
 $em = \App::getEm();
 foreach($xmlFiles as $xmlFile)
 {
 try
 {
 $feedObj = Parser::xmlToObject($xmlFile);
 ...import xml data to database - differns ...
 }
 catch(\Exception $e)
 {
 ....
 }
 }
}

Only the "..." area differs a lot because of the different XML structure but this part isn't really important for the question. In short: A Doctrine entity is created and many setAttributeX methods are called and in the end $em->flush().

My feeling tells me that it is not right to have three classes with only static methods to import XML files, but I can't find a better solution.

• Pinoniq: create an abstract class that is then extended by TypA, TypeB,...
• Manuel: There are no methods that I need in all classes (TypeA, TypeB, TypeC) therefore I could better use a Interface to ensure all needed function of a "Import"-Class are implemented. But I think the major problem is the "3 Classes with only static methods"-Solution.
• Robert Snyder: I agree with @Pinoniq (called him out since you didn't) If you make a abstract marker class that is extended by typeA, B,C you can make a few methods that all 3 share in the marker class. And you can make a GetXMLType (name??) that returns the correct instance of the Parser you need. That would clean up your code a bunch. Any common methods can be made in the Abstract class so you don't repeat your self.
• Manuel: @Robert Snyder Thanks for your feedback. The "problem" is that no methods are shared between the three classes. It's too individually. The only repeating tasks I would like to have in a Helper-Class because I also need them outside the import functionality. I thought my approach using three classes with only static methods was completly wrong but If i understand you correctly it's nothing wrong with it?
• Pinoniq: @Manuel Personally I try to avoid static methods since they make me right bad code. But that's because I tend to get very lazy sometimes. I also alway use dependency injection because that makes unit tests a lot easier. Just my 5 cents

Answer 1

I would go somewhere in the lines of the following code:

abstract class ObjectImporter {
 protected $em;
 public function __construct($em) {
 $this->em = $em;
 }
 abstract public function import($object);
}
class TypeAImporter extends ObjectParser {
 public function __construct($em) {
 parent::__construct($em);
 }
 public function import($object) {
 #do your import magic
 }
}
class XmlImporter {
 private $importer;
 public function __construct($importer) {
 $this->importer = $importer;
 }
 public function import($files) {
 foreach ( $files as $file ) {
 $obj = Parser::xmlToObject($file);
 $this->importer->import($obj);
 }
 }
}
switch ( $xmlType ) {
 case 'TypeA':
 $importer = new XmlImporter(new TypeAImporter(\App::getEm()));
 break;
}
$importer->import($files);

Some notes: the code is not ideal (names aren't that good imo) and I don't like the Parser::xmlToObject method. The Parser should be injected into the XmlImporter class aswel. This way its easier tested and you can also very easily change the xml Parser.

I hope the code is of any use.

• Manuel: Thank you for this example. Looks much tidier as mine. Although I actually do not need a individually class instance for this task because the processing is object independently but it seems more a question of what harms a instance instead of whats the benefit. Or I'm wrong?
• Pinoniq: It's more a question of how easy do you want to be able to run Tests on your code. At my work everything gets unit tested. even if I remove some white-spaces. That is why I use dependency injection instead of using static classes or using the 'new' operator inside a class. More info here: stackoverflow.com/questions/1580641/… for instance. So the benefit is the ability to write better an smaller tests ;)