Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Allow for stuff like:

  • API/ExampleObjects
  • API/ExampleObjects/SOME_INTEGER_ID/Children
  • ...

with children knowing their owners (aka parents).

Full repo here

  • Please be brutally honest if something can be improved!
  • I couldn't add the tag as I don't have 150 rep yet.

<?php

class RestApi {

    private 
        $id,
        $owner
    ;


    public function __construct( $params = false ) {

        if( $params['request'] ) {
            // grab first piece of the request
            if( strlen($request = trim(strstr($params['request'], '/'), '/')) > 0 ) {
                // php 5.2 compatible
                $pieces = explode('/', $params['request']);

                if( is_numeric($pieces[0]) ) {
                    $this->setId( $pieces[0] );
                    $params['request'] = $request;
                }
                unset( $pieces );
            } else {
                // if the request is numeric, then it's an id
                if( is_numeric($params['request']) ) {
                    $this->setId( $params['request'] );
                    $params['request'] = '';
                }
            }
        }   
    }


    public function getInstance( $params ) {                
        if( isset($params['owner']) && is_object( $params['owner'] ) ) {
            $this->setOwner( $params['owner'] );
        }

        // if resource starts with "api/", strip it
        if( strpos($params['request'], 'api/' ) === 0 ){
            $params['request'] = substr($params['request'], 4);
        }

        if( $params['request'] ) {
            if( strlen($request = trim(strstr($params['request'], '/'), '/')) > 0 ) {
                // php 5.2 compatible
                $pieces = explode('/', $params['request']);
                $class = $pieces[0];
                unset( $pieces );
            } else {
                $class = $params['request'];
            }

            $class = get_class($this) . $class;

            $instance = new $class(array('request' => &$request));  
            $instance->setOwner( $this );

            if( $request ) {
                return $instance->getInstance(array(
                    'request' => $request
                ));
            }

            return $instance;
        }
    }

    public function getOwner() {
        return $this->owner;
    }

    protected function setOwner( $owner ) {
        $this->owner = $owner;
    }

    public function getId() {
        return $this->id;
    }

    protected function setId( $id ) {
        $this->id = (int)$id;
    }


}
share|improve this question
1  
This is just a front controller. There is nothing inherently RESTful about it. – Paul Nov 25 '13 at 17:10
    
sorry @Paul I don't understand - could you please elaborate on what you mean by "inherently RESTful"? – Eva Nov 26 '13 at 6:30

1 Answer 1

Good

  • Private attributes for storing state in the class (id, owner).
  • Getter and setter methods are simple.

Bad

Not a RestApi

  • REST is an architectural style it can't be represented by an object. It is the wrong level of abstraction for an object to represent an architectural style.
  • Your RestApi object has public methods: getInstance, getId and getOwner. These have nothing to do with representational state transfer.

Constructor

  • The constructor should only create the object. By doing so much in your constructor you make it difficult to extend your class (going against the Open / Closed principle). It also makes it harder to unit test this code.
  • If you construct your object with an array that doesn't contain the key 'request' your code will emit an error notice.
  • unset($pieces); is unnecessary. Don't worry about memory management.

getInstance

  • This method should not be called getInstance.
  • This method does too much.
  • It can call setOwner when it should only be getting an instance.
  • Also, why should the class that you create depend on your RestApi object? get_class($this) will return 'RestApi' or 'RestApiDerived' if you derive another class from it. Why should the object that you create be so tightly coupled to the RestApi class?

I'm actually left confused by this class. The naming of methods and the object itself doesn't help me to understand what sort of objects this class represents. It doesn't map to anything in the real world that I can understand.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.