0
\$\begingroup\$

I have a class that takes a string or an integer, which stand for username or user id in the database. I am not comfortable with my constructor taking "a string or an integer" and want to modify it to be more type-specific. But how do I do so?

class User
{

    /**
     *
     * @var string|int
     */
    private $id;    

    /**
     *
     * @var array
     */
    private $udata;

    /**
     *
     * @param string|int $var            
     */
    function __construct($var)
    {
        $this->id = 0;
        $this->lookup($var);
    }

    /**
     *
     * @param string|int $var            
     */
    function lookup($var)
    {
        $sql = "SELECT * FROM user WHERE " 
            . (is_numeric($var) ? 'id' : 'username') . " = " . db_input($var);
        $row = query($sql);
        $this->udata = $row;
        $this->id = $row['id'];
        return $this->id;
    }
}

/**
 * Uses
 */
$user = new User(51);
$user = new User("JohnDoe");
\$\endgroup\$
2
\$\begingroup\$

Create static functions findById and findByUsername and clean-up User constructor or modify it to accept $id and $udata; you can also make it private, if you don't need it outside.

class User {
    // ...

    public static function findById (id) {
        // TODO
    }      

    public static function findByName (name) {
        // TODO
    }
}


$user = User::findById(51)
$user2 = User::findByName("JohnDoe")
\$\endgroup\$
1
\$\begingroup\$

I would agree with comments by @JurePolutnik to be explicit in the methods to expose. I would also suggest you perform some validation against parameters passed to public methods and perhaps even consider making your constructor private to force use of the static factory methods.

For example:

class User
{
    public static function getById($id)
    {
        if(!is_int($id) || $id < 1) {
            throw new InvalidArgumentException('Positive integer expected');
        }
        return new User($id);
    }

    public static function getByUsername($username)
    {
        if(empty($username) || !is_string($username) {
            throw new InvalidArgumentException('Non-empty string expected');
        }
        return new User($username);
    }

    // we make constructor private to force use of one of static methods
    private function __construct($search)
    {
        $lookupField = 'id';
        if(is_string($search)) {
            $lookupField = 'username';
        }
        // perform your query against $lookupField
        // then saturate your object
    }
}

I would recommend moving away from use of SELECT * queries. They make your code harder to read without having to go look at database schema (what fields do I have access to?). They can make your code fragile to table schema changes (you mean I broke something by adding a new column?). They can also make your application consume more bandwidth sending fields that are not being used back and forth (a great example of this are cases where you have timestamp/audit fields that are not meaningful to application).


I don't know exactly what query() and db_input() functions do here, but this sort of global function usage oftentimes hints at a bad design with regards to how your application interacts with database resources. Have you considered injecting the DB connection to this class?


Your class, as shown, doesn't really provide a method for getting at the user data. What is the point of instantiating a user object if you can't read any data out of it? Are you planning on implementing __get() to perform lookups against the array (something I would discourage in favor of proper properties)? How does a developer even know what properties, besides id are available on the user?


If you are going to use doc blocks (something I would encourage), you should, at a very minimum, include @return annotations for your public methods (but ideally for all methods - this helps you when working with the class in a modern IDE).


Why would your $id property be string|int it should only be an integer, right? If you want it to be an integer, you need to cast the value returned from DB, which will be a string value (absent any data bindings to the DB result).

\$\endgroup\$
  • \$\begingroup\$ thanks. To note I am using PHP7, which has type hinting, so some things can be trimmed, like !is_int($id), if I specify getById(int $id) \$\endgroup\$ – Dennis May 17 '17 at 14:04
  • \$\begingroup\$ On SELECT *: what if I have a general getUser method that can be used by many calling methods, requesting various different subsets of information from the User. Won't it make sense to have one such method rather than 5-6 that use the same SQL structure but request different column subsets? \$\endgroup\$ – Dennis May 17 '17 at 15:03
  • \$\begingroup\$ @Dennis What is your intent with this class? Does a user not a have a certain set of properties regardless as to how one chooses to instantiate it? If a caller only wants to get certain fields from a user, that is more a matter of how you set up getters as opposed to how you saturate the user object from the database, no? \$\endgroup\$ – Mike Brant May 18 '17 at 3:50
  • \$\begingroup\$ Not sure I understood. if I do select id, name from user; using database but omitting gender column, even if I have getGender() as my getter, that information will be missing. So it does matter how I saturate object from database, even if I set up my getters correctly. I need the glass to store/retrieve user information. Show name for a user. Get email to send them an email, etc. \$\endgroup\$ – Dennis May 18 '17 at 13:47
  • \$\begingroup\$ @Dennis Exactly. So when you saturate your object, you should always use a common SELECT statement against the DB such that the User object is fully saturated with all the data you might need. How calling code might need to get information from the object is a separate question, but really only a matter of what methods you want to provide to expose information about the user properties. You don't want to live in a world where you continually re-query the database for the same User object for all the different combinations of data different callers may need. You saturate the object once. \$\endgroup\$ – Mike Brant May 18 '17 at 15:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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