Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this annoying problem in PHP with faking overloading constructors. I understand the concept, but the code I produce feels kind of ugly. It isn't a lot of code, just bad code. Any ideas on how to make this sane?

/**
 * Instantiate Fencer object from user meta database.
 *
 * If fencer data does not exist in database and USFA ID is provided,
 * it will automatically update from API
 *
 * If USFA ID is not provided, will throw exception
 *
 * If API data is provided, a user ID must also be provided
 * that way we can save the data from the API to the user.
 *
 * @param int|null $user_id
 * @param string|null $usfa_id
 * @param array|null $raw_data
 *
 * @throws InvalidArgumentException
 *  1. If the user does not exist and USFA ID not provided
 *  2. Raw API data is provided, but not a User ID to save it to
 */
private function __construct( $user_id = null, $usfa_id = null, $raw_data = null ) {
    if ( $raw_data === null ) {
        if ( null === $user_id ) {
            $user_id = self::get_user_id_from_usfa_id( $usfa_id );
        }

        $this->wp_id = $user_id;

        $fencerdata = get_user_meta( $user_id, 'fence_plus_fencer_data', true );

        if ( ! empty( $fencerdata ) ) {
            foreach ( $fencerdata as $key => $data ) {
                call_user_func( array( $this, 'set_' . $key ), $data );
                // set all properties by calling internal setters based on fencer user meta data key
            }
        }
        else if ( $usfa_id != null ) {
            $this->usfa_id = $usfa_id;
            $this->update();
            $this->save();
        }
        else {
            throw new InvalidArgumentException( "Fencer data does not exist. Instantiate with USFA ID", 1 );
        }
    }
    else {
        if ( null == $user_id ) {
            throw new InvalidArgumentException( "User ID must be provided when instantiating with raw API data", 2 );
        }

        $this->wp_id = $user_id;
        $this->process_api_data( array( $raw_data ) );
        $this->interpret_data();
        $this->save();
    }
}
share|improve this question
add comment

1 Answer

It is a bad idea for a constructor to contain non-trivial code. Constructors should assign values to fields or another simple actions. If you need complex initialization, you should use factory object or factory method

share|improve this answer
 
Yes I understand that. The idea of the constructor is to make sure that all of the fields are populated, and that is what my constructor accomplishes. After the constructor runs, the object has the same state, regardless of how it was instantiated. This seems like a different problem, than one a factory method would solve, as the output is the same class. Am I missing something with my idea of a factory? –  TimothyBJacobs Oct 9 '13 at 16:25
 
The main problem is calling methods like these get_user_id_from_usfa_id and throwing exceptions in constructor. 1) it is not readable. When i call new i dont expect some db queries or any other side effects 2) when you call new object is created, but when you throw exception in constructor you dont get that object. I dont know how php handle this problem, but for example in C you wold have memory leak. 3) it is hard to test. If i want to test some method i have to mock some DB/net connection –  DominikM Oct 10 '13 at 6:19
1  
and 4) constructors are not part of the inheritance. So if you put some logic in construcotr you have to create constructor and call super constructor in it but there is no other way to invoke this logic if it is in constructor (sorry if it is in php, i am not familiar with php so much) –  DominikM Oct 10 '13 at 6:22
add comment

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.