HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Return over echo?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
returnoverecho

Problem

I have a number of functions inside class, which serve my scripts to provide functionality such as a login, which is the function I am going to be using as my examples.

public static function GetCredentials ($Argument_1, $Argument_2){
        $GetUserInformation = self::$Database->prepare("SELECT Username,Password FROM users WHERE Username=? ");
        $GetUserInformation->bind_param('s',$Argument_1);
        $GetUserInformation->execute();
        $GetUserInformation->bind_result($StoredUsername, $StoredPassword);
        $GetUserInformation->fetch();
         //Re-use Data
        $Search_Count = $GetUserInformation->data_seek(0);
        if ($Search_Count == 1){
            if ($StoredPassword !== $Argument_2){
                echo "Passwords Do Not Match!";
                exit;
            }
                $_SESSION['Username'] = $Argument_1;
                header('Location: Loggedin.php');
        }else{
            echo "User Does Not Exist";
        }
    }
}


I am planning to call this function once to perform the login for my user.

Now, do I echo through this function, or is it in best practices to return statements aswell as return false; to provide an error. then on my Login checking page perform:

if ($Foo::GetCredentials($_POST['Username'], $_POST['Password']) == false)
{
 echo $Foo:GetCredentials($_POST['Username'], $_POST['Password']);
}

Solution

Multiple problems

Why is static?
Why is the database hardcoded in the class?
Echo or return? Neither
Exit?
Direct $_SESSION write (super global)
HTTP stuff in an Auth class?

Why is static?

You are using stuffs in static where you should not have. Handling database through a static proxy? Untestable, unreadable and what happens if some forget initialize it?
Same thing applies to your class which is handling the authentacation. Why is it static? If you wan't to access it whereever you want then create a static facade class and keep the main logic in a separated non-static class.

Why is the database hardcoded in the class?

What happens if you don't want to use anymore a standard SQL database to store your users? You will rewrite the whole class to achive that?
If you create a new class as i described above leave out this hardcoding use constructor injection instead:

class Auth {

    public function __construct(IUSerStore $store) {
        /* store reference, etc ... */
    }

}

interface IUSerStore {

    function GetUserByName($name);

}


In this way you have no string dependency to a SQL database to query user you can have for example an in memory store for users if you want to test the Auth class.

Echo or return? Neither

Currently you are echoing text from a business logic layer which is bad really bad. Only in the presentation layer should be print out stuff to the end user.
The return would not be the solution for this becouse you can't tell what was the Auth process result (only from the message) so create a result class:

class AuthResult {

    public $Success;
    public $Message;

}


This is a really simple approach but it can be used to signal what happaned in the authentication process.

Exit?

What is the reason of the exists of an exit statement in a business layer class? Why is stopped the whole request process if the passwords not match? What happens if you want to test the password equality check method? If they are not equal the test stops?

Direct $_SESSION write (super global)

Of course writing into the $_SESSION array is not bad you can have your own session data handler in the background so everything can be done this way. But again: what happens if you want to test the functionality of writing a session data in your Auth class? You will test the session data saver also (your own or the one built into PHP)?
I would wrap it into a class and pass an argument to the Auth class as i described above the IUserStore solution.

HTTP stuff in an Auth class?

Why is a header() call in your Auth class? Is it really neccessary or the redirection can be done depending on the result of the authentication process in your controller if you are following an MVC approach?

Code Snippets

class Auth {

    public function __construct(IUSerStore $store) {
        /* store reference, etc ... */
    }

}

interface IUSerStore {

    function GetUserByName($name);

}
class AuthResult {

    public $Success;
    public $Message;

}

Context

StackExchange Code Review Q#24580, answer score: 4

Revisions (0)

No revisions yet.