2

I have an SystemInfoFactory class in my application which has a getSystemInfo() method:

/**
 * Returns SystemInfo object based on which OS
 * server uses
 *
 * @return SystemInfoInterface SystemInfo object
 */
public function getSystemInfo()
{
    $os = $this->getOS();

    $systemInfo = null;

    switch ($os) {
        case "Linux":
            $systemInfo = new LinuxInfo();
            break;
        case "Darwin":
            $systemInfo = new OSXInfo();
            break;
        case "Windows":
            $systemInfo = new WindowsInfo();
            break;
    }
    return $systemInfo;
}

So it chooses appropriate object according to host system. Now, each "info class" implements SystemInfo interface (methods like getArchitecture, getCPU etc.), but as you see, nowhere in my code is it checked whether returned object really implements interface. Would it be considered "good practice" to check if selected $systemInfo object implements it before returning it? It obviously isn't required, but in case somebody extends this application (adds BSD support for example) and forgets to implement all methods it might be harder for him to debug.

Improve this question
4
  • Shouldn't PHP throw an error when somebody uses an Interface but doesn't implement all methods?? Commented Mar 5, 2013 at 14:12
  • It will, but the fact is, he can just implement all interface methods without explicitly defining implements SystemInfoInterface in his class and the code will work without problems. Commented Mar 5, 2013 at 14:13
  • Currently, he could just simply forget to implement one of the methods and application will throw an undefined method error at some point, but the real problem source could be buried deep in call stack. Commented Mar 5, 2013 at 14:16
  • Yeah, that's a problem with PHPs dynamic typing. Not sure how to solve that. Commented Mar 5, 2013 at 14:16

2 Answers 2

Reset to default
2

Absolutely it would be good practice. You are defining in your docblock that your method returns an instance of SystemInfo. Your callers should be able to rely on that. This is simple in your code:

/**
 * Returns SystemInfo object based on which OS
 * server uses
 *
 * @return SystemInfoInterface SystemInfo object
 */
public function getSystemInfo()
{
    $os = $this->getOS();

    $systemInfo = null;

    switch ($os) {
        case "Linux":
            $systemInfo = new LinuxInfo();
            break;
        case "Darwin":
            $systemInfo = new OSXInfo();
            break;
        case "Windows":
            $systemInfo = new WindowsInfo();
            break;
        default:
            throw new \RuntimeException('System not supported');
            break;
    }

    if (!$systeminfo instanceof SystemInfo) {
        throw new \RuntimeException('Invalid SystemInfo object returned');
    }

    return $systemInfo;
}

Make sure you declare that you'll be throwing exceptions from this method call. The exception here makes it very clear what's going on, rather than having to chase through an "undefined method" error later on in the code.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

1

I think, the keyword in here is Duck Typing. The interface of an object is defined by its methods and attributes rather than its ancestors and implemented interfaces.

Have a look at this: http://en.wikipedia.org/wiki/Duck_typing

Back to PHP: It's perfectly valid and no bad style if you do not check if the object implements your interface. If the code crashes the implementor of the class has to be blamed. I would find it annoying if the code is messed up with if ($obj instanceof FancyInterface) {.

Improve this answer

7 Comments

You're propagating horrible coding practices. The method indicates it's going to return an object of a certain type. If it doesn't, it's bad code. Period. End of story. Why in the world would you have interfaces if not for these reasons? Always, always, always code to interface. Not implementation.
Of course one should code against an interface. But if a class is supposed to implement an interface I expect it to do that. I don't want to have to check every object if it implements certain methods or interfaces. I expect them to do what they promise they can. That's one benefit of Duck Typing.
That's an expectation that'll cost you a lot of wasted debugging time that 3 additional lines of code could have solved. Note that this is only even necessary for factory-based methods due to their variable return values. Otherwise, you can simply type-hint your method arguments with the interface and it only costs you a few characters.
Type hinting is, where applicable, very great. Of course, defining object types in docblock is a must too. Checking for interfaces at central parts of the software, like factories, only is OK for me too. But, hell, i can not agree to checking for interfaces at all possible parts of classes, scripts, whatever. I understood your answer that way because you didn't mention it the way you did it here in the comments. I would go that far to only check with instanceof where third party libs are part of the game - whether i deliver objects to 3rd party or i accept objects from 3rd party.
If a method is returning one object, and it's controlled by the developer, then not checking it's implementation of an interface is acceptable. My argument is that in a factory method such as this (which may, at some point, be changed to a more abstract factory approach), when the OP said that others may extend and add support for other platforms or add new objects, I think that it's a cheap and reasonable check to make.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.