1
\$\begingroup\$

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();
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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

\$\endgroup\$
3
  • \$\begingroup\$ 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? \$\endgroup\$ Oct 9, 2013 at 16:25
  • \$\begingroup\$ 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 \$\endgroup\$
    – DominikM
    Oct 10, 2013 at 6:19
  • 1
    \$\begingroup\$ 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) \$\endgroup\$
    – DominikM
    Oct 10, 2013 at 6:22

Your Answer

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

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