Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

When a user is created, deleted and update the system will give feed back. e.g. User Deleted/Updated/Create success or unsuccessfully, but I'm sure there is a better way to do it than what I have done because I repeat the same code in all functions within my class.

Code to create user

$userOne = new userActions($database, 'Greg', '324b2643243');//
echo $userOne->create();

Code to delete user

$userOne = new userActions($database, 'Abel'); //
echo $userOne->delete();

Code for user creation

public function create()
{//Works
    $outPut = "";
    if(!$this->user_exists)
        {
            list($password_hash, $salt) = $this->hash($this->password);
            echo 'Hash: ' . $password_hash . '<br>' . 'Salt: ' . $salt . '<br />';
            $result = $this->connection->query("INSERT INTO users VALUES( NULL, '{$this->username}' , '{$password_hash}' , '{$salt}' ) ");

            if($result)
                {
                    $outPut = "User has been created";
                }
                else
                {
                    $outPut = "User could not be created";
                }

        }
        else
        {
            $outPut = 'Username already exists';
        }

    return $outPut;
}

Is there a better way to display feedback to the user istead of having $outPut every time in all of my create, delete and update functions?

share|improve this question
You could make them static functions. echo userActions::create($database, 'Greg', '324b2643243'); If you don't use them for something else. – AlucardTheRipper Apr 14 at 11:40

1 Answer

up vote 1 down vote

I recommend splitting your model (the database operation) from your view (the localization of your message).

Depending if you prefer a return code or a exception flow your code could look like:

public function create()
{
    if($this->user_exists) return ""; //do you really want to show nothing?
    $error=UserModel::create($this->password,$salt,$this->username); // don't have to be static! It's up to you.
    if ($error==-1) return "User could not be created";  //could be a constant UserModel::ERROR_GENERAL
    if ($error==0) return "Username already exists"; //UserModel::ERROR_EXISTS
    return "User has been created";
}

Translating this to exception is up to the reader :)

Actually I even would prefer to give the $error code to my template and let it handle the localization:

public function create()
{
    if($this->user_exists) return "";
    $error=UserModel::create($this->password,$salt,$this->username); 
    $view=new View("templatefile.php");
    $view->assign('error',$error);
    return $view->render();
}

//"templatefile.php"
<?if ($error==1):?>...
<?elseif ($error==0):?>...
<?else:?>...
<?endif?>
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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