Return over echo?

Question

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']);
}

Answer 1

The short answer in my opinion is: Do not echo, return a meaningful response.

@Peter Kiss nailed it in so many ways and I would like to map some of his thoughts to well known programming principles and design patterns.

Why is the database hardcoded in the class?

Single Responsibility Principle:

Each class, function, module should have one and only one reason to change. Robert C. Matrin in his book Clean Code Ch3 explains

FUNCTIONS [classes, modules] SHOULD DO ONE THING. THEY SHOULD DO IT WELL. THEY SHOULD DO IT ONLY.

In your code you have DB queries, auth checks, messages intended to be displayed to the user (presentation layer), and redirection. Avoid this kind of mess by creating classes that specialize by identifying what they are responsible of.

What happens if you don't want to use anymore a standard SQL database to store your users?

Open/Closed Principle:

your classes, functions, modules, packages, bundles should be open for extension, and closed for modification. What this means is that the behavior of your class can be modified without altering the code. A simple example would be a class Shape. The behavior of Shape could be modified by creating a new class Rectangle that extends from Shape. See other examples of this principle here and here. Another example of extending functionality is Constructor Injection as described by @Peter Kiss, this is part of a Design Pattern called Dependency Injection explained below.

If you create a new class as i described above leave out this hardcoding use constructor injection instead

Dependency Injection (DI):

Dependency injection means giving components the dependencies they need at run time through their constructor, methods, or directly into fields. See this example/tutorial in php for more information and learn how to use Inversion of Control (IoC) to simplify the use of your class. Also take a look at Pimple: a simple Dependency Injection Container.

Echo or return? Neither

While I agree that you shouldn't return an message that mixes the presentation layer with the logic, I believe is important to clarify that returning something meaningful is essential based on what you want to accomplish: get user credentials. This brings us to the next principle.

Command Query Separation (CQS):

it states that your methods should either perform an action or return to the caller the results of a query. In your case you are not performing an action such as authentication, but rather you want to query the user credentials. Return a UserCredentials object, that is responsible to set the appropriate info in a session via a cookie or key-value data store like Redis or Memcached. By using the proper patterns and abstractions you can even allow the UserCredentials class set the session using the desired method!

Exit?

Prefer Exceptions to returning error codes/messages:

You can address your two error cases ("Passwords Do Not Match" and "User Does Not Exist") cleanly by throwing meaningful exceptions and force the user of the class deal with the error immediately. This way you also avid a sudden termination of the logic in your class. Also avoid using the generic Exception class only, create Exception classes that extend from Exception like:

class AuthenticationException extends Exception{};

Make it meaningful :)

Final thoughts

Use an MVC framework. A framework will help you separate the business logic from the presentation they also implement common design patterns, and provide classes to help you deal with common task such authentication and sessions. I think CodeIgniter is a great lightweight framework to learn MVC, but I would transition out to a micro framework such as Silex other PHP 5.3+ frameworks like FuelPHP, Laravel4, or enterprise level frameworks like Symfony2 or Zend Framework 2. It all depends on the size of your project.

Stand in the shoulders of other developers. Use composer to install and load third party libraries that can be easily integrated to almost any project. See https://getcomposer.org/ and https://packagist.org/ for more info.

I recommend getting the book Clean Code by Robert C. Matrin. While I do not agree with some of his views, he has lots of helpful principles that will point you in the right direction to write great, extensible, readable code.

Answer 2

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?