2
\$\begingroup\$

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.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ create an abstract class that is then extended by TypA, TypeB,... \$\endgroup\$
    – Pinoniq
    Jul 4, 2013 at 18:45
  • \$\begingroup\$ 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. \$\endgroup\$
    – Manuel
    Jul 5, 2013 at 12:44
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Jul 5, 2013 at 13:23
  • \$\begingroup\$ @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? \$\endgroup\$
    – Manuel
    Jul 5, 2013 at 13:54
  • \$\begingroup\$ @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 \$\endgroup\$
    – Pinoniq
    Jul 5, 2013 at 14:00

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ 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? \$\endgroup\$
    – Manuel
    Jul 5, 2013 at 14:33
  • \$\begingroup\$ 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 ;) \$\endgroup\$
    – Pinoniq
    Jul 5, 2013 at 15:29

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.