What would you change in my code for best practices/maintenance?

Question

I've got a small snippet of code below and I was curious what types of things you would change with regards to best practices/code maintainablity and so on.

function _setAccountStatus($Username, $AccountStatus)
{
if ($Username == '' || ($AccountStatus != 'Active' || $AccountStatus != 'Banned' || $AccountStatus != 'Suspended')) {
    // TODO: throw error here.
}

$c1 = new Criteria();

$c1->add(UsersPeer::USERNAME,$Username);

$rs = UsersPeer::doSelect($c1);

if (count($rs) > 0) {
    $UserRow = array_pop($rs);

    $UserRow->setAccountStatus($AccountStatus);

    try {
        $UserRow->save();
    } catch ( PropelException $e ) {
        return false;
    }

    return true;
}

return false;
}

Answer 1

I would use the empty() instead of $Username == '' in your if statement. I haven't used propel before, but I would prefer to have this method be on my User object itself with the fetching and saving of the user object performed by a seperate object. Pseudo code would be something like this.

$user = userManager->getUser($username); $user->setAccountStatus($accountStatus); $userManager->saveUser($user);

Answer 2

An else clause before the last return false would be prefererred, just to make the code more readable.