CODEWITHSUNDEEP
phpoopdesign-patterns
Antoine
Antoine

Reputation: 500

Parser for multiple formats and multiple operations (design pattern available ?)

I am currently writing a text file parser for several formats. While the text file is parsed, there are different kinds of operations to do. I am trying to do something clean using OOP. Here is where I am stuck : 

abstract class Parser
{
    abstract protected function DoSomeStuff($data);

    public function Parse($src)
    {
        if ( $this->GetFormat($src) == 1 )
        {                       
            $data =$this->GetSomeDataFromFormat1($src);
            DoSomeStuff($data);
        }
        if ( $this->GetFormat($src) == 2 )
        {
            $data = $this->GetSomeDataFromFormat2($src);
            DoSomeStuff($data);
        }
    }   
}

class DoSomething extends Parser
{
    protected function DoSomeStuff($data)
    {
        // Doing some stuff with data
    }
}

class DoSomethingElse extends Parser
{
    protected function DoSomeStuff($data)
    {
        // Doing some other stuff with data
    }
}

$ds = new DoSomething();
$ds->Parse(...);

$dse = new DoSomethingElse();
$dse->Parse(...);

As you can see : all the code for all files formats is in class Parser. What can I do to make this cleaner ?

Thanks Antoine

Upvotes: 0

Views: 1995

Answers (3)

Matt Ball
Matt Ball

Reputation: 359936

It sounds like the Strategy pattern might help clean this up.

See also: StackOverflow: [java] strategy pattern search

Upvotes: 2

Barkermn01
Barkermn01

Reputation: 6842

GetFormat returns and int this is not easy for a user to understand what int is what it would be best if you returned some thing like "CSV" or "XML"

Another is 

if ( $this->GetFormat($src) == 1 )
{
}
if ( $this->GetFormat($src) == 2 )
{
}

//This should be a switch
$src = $this->getFormat($src);

switch($src){
    case 1:

    break;
    case 2:

    break;
}

This then prevents the code from loading a method twice when it can do it once, and a switch is less memory than an if

and the last i can spot is

GetSomeDataFromFormat1 and GetSomeDataFromFormat2

This might be to the code you have given us but an Abstract class should only ever call other Abstract methods or methods that are provided inside that class they should require the user to know they have to add 2 method's other than i like your code it's easy to read which is the main thing in coding

Upvotes: 0

simshaun
simshaun

Reputation: 21466

It's generally a good idea to follow a 1-class-per-file rule.

Other than that, there isn't a whole lot more you can do to clean it up.

The only other thing to decide on is how you want to load each class file, which can be done several ways. I'd suggest making a factory class with a registry.

Upvotes: 0

Related Questions