usernameAvailable method of Model class - good design?

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

usernameAvailable method of Model class - good design?

Post by Luke »

Something about this method smells funny to me:

Code: Select all

public function usernameAvailable($username)
    {
        $db = $this->DB();
        $result = $db->execute("
            SELECT `id`
            FROM " . $this->_table . "
            WHERE `username` = " . $db->qstr($username)
        );
        if (!empty($result->fields))
        {
            // Check if current username is same as username we're checking
            if ($this->id)
            {
                return ($this->username == $username); 
            }
        }
        return true;
    }
I use it to check if a username exists so I can then display a "username already exists" error to the user or admin when setting up a new member account. I am using it for both the "add member" and "edit member" forms. So on the add form, it only needs to check that the username doesn't exists, but on the edit screen, it needs to check that the username doesn't exists or that it is the username already given to the user.

Would it make more sense to just not apply the validation rule (This method) unless the username has been changed?
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

The function name doesn't match the code, or rather one part of the code doesn't match the function name. Specifically the return where usernames match.
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

shouldn't that last return return false?

In fact the entire second half of the function might be shortened to:

Code: Select all

if (!empty($result->fields)) return true;
return false;
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Code: Select all

public function usernameAvailable($username)
{
   return count($this->DB()->execute('SELECT `id` FROM '.$this->table.' WHERE `username` = \''. $db->qstr($username).'\'') > 0);
}
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

nice 8)
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

feyd wrote:The function name doesn't match the code, or rather one part of the code doesn't match the function name. Specifically the return where usernames match.
I agree... I changed it to this:

Code: Select all

public function usernameAvailable($username)
    {
        $db = $this->DB();
        $result = $db->execute("
			SELECT `id`
			FROM " . $this->_table . "
			WHERE `username` = " . $db->qstr($username)
		);
	return empty($result->fields);
    }
And then where it's called, I first check that the username has been changed from the original before checking if it is available:

Code: Select all

// Check if current username is same as username we're checking
if ($user->username != $form['username']->getValue())
{
    $form->addRule('username', 'Username "' . $username->getValue('username') . '" is already in use', 'callback', array($user, 'usernameAvailable'));
}
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

I think my question is: what class this method is in? If it is in the User class then it shouldn't it be something like:

Code: Select all

$user = new User($username);
if ($user->isAvailable()) {
(#10850)
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

arborint wrote:I think my question is: what class this method is in? If it is in the User class then it shouldn't it be something like:

Code: Select all

$user = new User($username);
if ($user->isAvailable()) {
eh, I guess that depends on if you think $user->isAvailable makes more sense than $user->usernameAvailable(). to me, user and username aren't synonymous.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

If you wanted to be lazy, and you were dealing with a fully fledged model, you could just blindly create the User object, attempt to add it to the database, and then gracefully back out (using try...catch) when the DB complains about a unique key violation (assuming username has been appropriately defined). Saves you an SQL query, although it's a little voodoo-ish. If you think about it, though, it makes intuitive sense.
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

yea that's not a bad idea...
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

arborint wrote:I think my question is: what class this method is in? If it is in the User class then it shouldn't it be something like:

Code: Select all

$user = new User($username);
if ($user->isAvailable()) {
Christopher, did you mean it shouldn't or shouldn't it? Makes a big difference ...
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

matthijs wrote:Christopher, did you mean it shouldn't or shouldn't it? Makes a big difference ...
I was supposed to be shouldn't it. And it wasn't the name so much as the parameter that I meant to point out in my example -- isThisUsersUsernameAvailable(). The user should know about itself, otherwise you could just make a procedural helper function.

I might design it with a UserFactory that returned a valid User object or null/error/exception if the requested username was not available.
Last edited by Christopher on Tue Jan 02, 2007 3:52 pm, edited 1 time in total.
(#10850)
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

arborint wrote:
matthijs wrote:Christopher, did you mean it shouldn't or shouldn't it? Makes a big difference ...
I was supposed to be shouldn't it. And it wasn't the name so much as the parameter that I meant to point out in my example -- isThisUsersUsernameAvailable()
oh, well in that case, yes
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

My original thinking was that you would create a new User object with a given username and the ask it if it was valid.
(#10850)
User avatar
Luke
The Ninja Space Mod
Posts: 6424
Joined: Fri Aug 05, 2005 1:53 pm
Location: Paradise, CA

Post by Luke »

yea I see what you mean now... you're right.
Post Reply