Is it correct for an object to reject it self?
I want to parse several files formats. I am wondering if it's correct OOP to "take the risk" to create an object for nothing.
class ParserFactory
{
private fn;
public function ParserFactory(fn)
{
this->fn = fn;
}
public function getParser()
{
a = new FormatAParser(this->fn);
if ( a->isValid() )
{
return( a );
}
b = new FormatBParser(this->fn);
// ... and so on...
}
}
class FormatAParser
{
/*
The object is telling us if is able to continue to work...
**CLEAN OR DIRTY DESIGN ?**
*/
public function isValid()
{
header = SomeConversionAndReadingStuff();
if ( header != "formatA" )
{
return(false)
}
return(true);
}
public function parse()
{
/*
Do the parsing, using the conversion stuff done in isValid
*/
}
}
Thanks
EDIT
I had very good answers. So, it is OK for an object to check its own validity. Anyway, my code was un-OOP because of the procedural way I select a parser (format detection). To improve this, it is better to user a Factory Pattern like this one (PHP code) :class Parser
{
protected $raw;
public function setRaw($raw)
{
$this->raw = $raw;
}
}
class ParserA extends Parser
{
public function __construct()
{
echo "ParserA constructor\n";
}
public function isValid()
{
if ( $this->raw == "A" )
{
return( true );
}
return(false);
}
}
class ParserB extends Parser
{
public function __construct()
{
echo "ParserB constructor\n";
}
public function isValid()
{
if ( $this->raw == "B" )
{
return( true );
}
return(false);
}
}
class ParserFactory
{
static private $parserClasses = array();
public static function registerParser($parserClassName)
{
self::$parserClasses[] = $parserClassName;
}
public static function getParser($raw)
{
foreach( self::$parserClasses as $parserClass )
开发者_StackOverflow {
$parser = new $parserClass();
$parser->setRaw($raw);
if ( $parser->isValid() )
{
return( $parser );
}
}
}
}
ParserFactory::registerParser("ParserA");
ParserFactory::registerParser("ParserB");
ParserFactory::getParser("B");
A more common design is
public function getParser()
{
if (FormatAParser::IsRecognizedHeader(this->fn)
{
return new FormatAParser(this->fn);
}
if (FormatBParser::IsRecognizedHeader(this->fn)
{
return new FormatBParser(this->fn);
}
}
This uses a class, not an object method to check the header. An obvious variation is to collect the different parsers in a list, instead of manually looping over them like this. This requires the ability to add classes (not objects) to a list.
I do not believe there is anything un-OO about having an object understand and evaluate its own validity. You could always make the isValid method static and pass in the 'criteria' (in this case the header. That way you don't have to instantiate the parser until you know you need it.
I think the possibly un-OO-ness here is coming from the fact you have multiple if statements, each apparently checking for a type of some sort (in this case a type of file). This jibes with our notion that there should probably be a class that represents each file type; this sort of switching is normally a code smell that something in the design isn't quite right. Practically, however, I think your solution is ok, but that MSalters is better, because it's hard to know how else a parser would work, as the file itself can't autogenerate a class instance for you.
精彩评论