5
\$\begingroup\$

I'm working on a module that downloads a file saves it then validates if the file contents are valid.

For simplicity lets assume that the file consists of 3 segments: Header, Body & Footer.

Now here is the design I thought of:

class File{
    private $header;
    private $body;
    private $footer;

    function  __construct($pathToFile){
        $this->header= new Header($this->getFileHeader());
        $this->body= new Header($this->getFileBody());
        $this->footer= new Header($this->getFileFooter());
    }
/* getFileHeader(), getFileBody(), getFileFooter() are private functions of the File class 
that basically reads the contents of the corresponding segment in the file. */

    function isValid(){
        if(!$this->header->isValid()) return false;
        if(!$this->body->isValid()) return false;
        if(!$this->footer->isValid()) return false;
    }
}

Header Segment class(I'll only show this one as the Body segment and Footer segment have similar structure)

class Header{
    private $content;
    private $validators=array();

    function  __construct($content){
        $this->content= $content;
        $this->validators= array(new NotEmptyValidator($content), new AllNumbersValidator($content)); 
        //for simplicity the validators will be only 2 and set in the constructor.
    }

    function isValid(){
        foreach($this->validators as $validator){
             if(!$validator->isValid()) return false;
        }
        return true;
    }

}

Finally the Validator class:

class NotEmptyValidator{
    private $content;

    function  __construct($content){
        $this->content= $content;
    }

    function isValid(){            
         return (empty($this->content));
    }

}

Questions:

  1. Overall does this look like a good object oriented design? Can it be improved?
  2. The Header class has an array of validators. I'm not sure if this is the best approach as I borrowed this idea from Zend framework form validators. Is there a better implementation?
  3. Am I over delegating the task of validation?

Update:

I've simplified the design and striped many details to focus mainly on the validation process.

Another point The file I'm trying to validate is an EDI FACT file, so I suppose the file downloaded will have to be parsed first then validated.

Maybe reading the file contents in the File class is a bad idea and I should create a class Parser that takes the downloaded file and then create the File object itself then comes the validation.

\$\endgroup\$

migrated from programmers.stackexchange.com Jul 18 '12 at 8:28

This question came from our site for professionals, academics, and students working within the systems development life cycle.

2
\$\begingroup\$

That isn't OOP, that's procedural, with a thin wrapping that looks like OOP. OOP will require that your validation be done on objects that share data and behavior.

Depending upon your file, this may or may not make sense.

OTOH, good procedural code is definitely better than the common PHP "just run it all together inside a single page" that frequently gets used.

EDIT: Expanding my answer after a comment.

First off, I should be clear that I wasn't criticizing your existing code, just saying they don't make it OOP. OOP is about data and behavior not just having things called classes and constructors.

Secondly, EDI probably means transactions of different types with well definined behavior for the various types -- that would be an opportunity to use/benefit from OOP. I would try to design a base class or interface that can be applied to all lines/transactions and then have derived classes for the header/footer and body lines.

You need to focus on the similaririty of your data and your process for handling/validating it, your OOP implemention will come out of that. It's a little to early to be asking about OOP.

\$\endgroup\$
  • \$\begingroup\$ Well, I'm trying to validate an EDI FACT file. Any suggestion on how to improve the classes? \$\endgroup\$ – Songo Jul 13 '12 at 1:17
  • \$\begingroup\$ Oh! I get your point and I agree with you. It seems I wasn't clear enough in my question, but I was hoping to improve the design mainly for the validation process . Actually I striped many functions and data from the classes to make it focus on the validation process :) \$\endgroup\$ – Songo Jul 13 '12 at 13:56
  • \$\begingroup\$ @Songo: well the validation and parsing are pretty much the same. Your current design is, as I said procedural. You should think of the file as a collection of lines. So, no header, footer, body. Just a collection, each object having an isValid property. The type of object would be determined when the line/s is parsed (unrecognized being always invalid). So, your constructor for the file should take either a path or a stream (or database id later) and then create your collection of objects as it reads the file. \$\endgroup\$ – jmoreno Jul 13 '12 at 15:49
  • \$\begingroup\$ But what if the File has a predefined structure that must contain a header segment followed by a collection of line (their number can vary) then a footer segment? I think separating them into 3 variables makes more sense IMHO. \$\endgroup\$ – Songo Jul 13 '12 at 20:59
  • \$\begingroup\$ @Songo: Why would you want the header/trailer excluded from your list of objects in the file? You might want/need to have a reference to the first and last items seperate from the list, but there's no reason why they shouldn't share an interface with the other items, and be part of the same collection. You seem to be headed towards an Interface that would allow you to check the validity of the file, but you haven't made it explicit by declaring either an Interface or an Abstract class with the isValid property. Making it explicit is useful, even in duck-typing languages. \$\endgroup\$ – jmoreno Jul 14 '12 at 1:08
0
\$\begingroup\$

Right off the bat, I'd consider getting the file reading for header, body, and footer out of your constructor. Not only are these potentially time-consuming operations, they're also quite likely to fail if you've got a mal-formed file. You'll want to think about whether you'd consider that a "validation failure" or another sort of structural failure altogether.

Beyond that, I'd consider how you'll handle interaction among header, footer, and body. For example, you might find that you've got a footer validation that depends on a checksum derived from body fields, though there are plenty of simpler validations that you could get away with evaluating sequentially.

\$\endgroup\$
  • \$\begingroup\$ hmmmm so you propose that I remove the File's constructor and create a public method like loadFile() instead that does the same job of the constructor? or should I create a loadHeader(), loadBody() & loadFooter()? \$\endgroup\$ – Songo Jul 12 '12 at 22:29
  • \$\begingroup\$ Maybe a load that takes a stream of some sort -- you'd like to pass in an open file handle rather than opening and closing the same file over and over, right? \$\endgroup\$ – D. Lambert Jul 13 '12 at 5:07

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.