Page 1 of 2
usernameAvailable method of Model class - good design?
Posted: Tue Jan 02, 2007 1:18 pm
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?
Posted: Tue Jan 02, 2007 1:43 pm
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.
Posted: Tue Jan 02, 2007 1:46 pm
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;
Posted: Tue Jan 02, 2007 1:51 pm
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);
}
Posted: Tue Jan 02, 2007 1:53 pm
by Kieran Huggins
nice

Posted: Tue Jan 02, 2007 1:53 pm
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'));
}
Posted: Tue Jan 02, 2007 2:21 pm
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()) {
Posted: Tue Jan 02, 2007 2:29 pm
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.
Posted: Tue Jan 02, 2007 2:44 pm
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.
Posted: Tue Jan 02, 2007 2:56 pm
by Luke
yea that's not a bad idea...
Posted: Tue Jan 02, 2007 3:22 pm
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 ...
Posted: Tue Jan 02, 2007 3:50 pm
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.
Posted: Tue Jan 02, 2007 3:52 pm
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
Posted: Tue Jan 02, 2007 3:54 pm
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.
Posted: Tue Jan 02, 2007 4:00 pm
by Luke
yea I see what you mean now... you're right.