1
\$\begingroup\$

I've searched plenty on this topic and have gotten a lot of good (but different) results. Some of the results weren't quite related and it does seem to be a matter of preference in the end, but I'm interested in if I'm following good design principles or not.

If this is too vague of a question, feel free to delete it, but can you recommend where I post it instead?

Also, this is just an example. There are quite a few things in here I would normally do differently but for the sake of simplicity, I did it this way.

The code is long, but you should be able to just copy & paste it directly into a single new PHP file and run it in your environment; there is no setup required.

Specific questions

  • Is this the correct way to use exceptions and handle them on the caller side?
  • Should I even be using exceptions for this?
  • Are my skeleton custom exceptions correct?

Code

You can view a copy in a separate window here. I will paste it here. Save it and run it in your environment, it should work as-is without any modifications:

Beware: long code ahead

<?php

    error_reporting ( E_ALL | E_STRICT );


    class MemberLoginException extends Exception
    {
    }


    class AccountsInsertException extends Exception
    {
    }


    class AccountsManager
    {

        protected $_accounts = array ();
        protected $_lcUsernames = array ();     # all usernames in lowercase for checking if username is taken


        public function __construct ( array $accounts = null )
        {
            $this->setAllAccounts ( $accounts );
        }


        public function __destruct ()
        {
            unset ( $this->_accounts, $this->_lcUsernames );
        }


        public function __toString ()
        {
            $return = '';

            if ( count ( $this->_accounts ) > 0 ) :

                $return = '<table>';
                $return .= '<tr><th>Username</th><th>Password</th></tr>';

                foreach ( $this->_accounts as $account ) :

                    $return .= 
                    '<tr>
                        <td>'. htmlentities ( $account['username'], ENT_QUOTES, 'UTF-8' ) . '</td>
                        <td>'. htmlentities ( $account['password'], ENT_QUOTES, 'UTF-8' ) . '</td>
                    </tr>';

                endforeach;

                $return .= '</table>';

                return $return;
            endif;
        }


        public function Clear ()
        {
            $this->_accounts = array ();
            $this->_lcUsernames = array ();
        }


        public function Authenticate ( Member $member )
        {
            $username = strtolower ( $member->getUsername () );

            if ( count ( $this->_accounts ) ) :

                foreach ( $this->_accounts as $account ) :

                    if ( strtolower ( $account['username'] ) == $username )
                        return ( bool ) ( $account['password'] == $member->getPassword () );

                endforeach;

            else :
                return false;
            endif;
        }


        public function getAllAccounts ()
        {
            return $this->_accounts;
        }


        public function setAllAccounts ( array $newValue = null )
        {
            if ( is_null ( $newValue ) )
                $this->_accounts = array ();
            else
                $this->_accounts = $newValue;
                $this->_lcUsernames = array ();

                foreach ( $this->_accounts as $account )
                    $this->_lcUsernames[] = strtolower ( $account['username'] );

            return $this;
        }


        public function hasAccount ( $username )
        {
            return in_array ( strtolower ( $username ), $this->_lcUsernames, false );
        }


        public function AddAccount ( $username, $password )
        {

            /*
            Faster to be redundant by storing a lowercase copy of the username for comparison

            if ( array_key_exists ( strtolower ( $username ), array_change_key_case ( $this->_accounts ) ) )
                throw new AccountsInsertException ( 'Unable to create account; account already exists.' );
            */

            if ( $this->hasAccount ( $username ) )
                throw new AccountsInsertException ( 'Unable to create account; account already exists.' );

            $this->_accounts[] = array (
                'username' => $username,
                'password' => $password,
            );

            $this->_lcUsernames[] = strtolower ( $username );
            return $this;
        }


        public function RemoveAccount ( $username )
        {
            if ( $this->hasAccount ( $username ) ) :
                unset ( $this->_accounts[$username] );
                unset ( $this->_lcUsernames [ strtolower ( $username ) ] );
            endif;

            return $this;
        }


        public function __Debug ()
        {
            echo "\r<pre>\r";
            print_r ( $this->_accounts );
            echo "\r</pre>\r\r\r<pre>\r";
            print_r ( $this->_lcUsernames );
            echo "\r</pre>\r\r";
        }

    }




    class Member
    {

        protected $_username = '';
        protected $_password = '';


        public function __construct ( $username, $password )
        {
            $this->setUsername ( $username );
            $this->setPassword ( $password );
        }


        public function getUsername ()
        {
            return $this->_username;
        }


        public function setUsername ( $newValue )
        {
            $this->_username = ( string ) $newValue;
            return $this;
        }            


        public function getPassword ()
        {
            return $this->_password;
        }


        public function setPassword ( $newValue )
        {
            $this->_password = ( string ) $newValue;
            return $this;
        }

    }



    # create a new accounts manager which stores all accounts and handles authentication
    # the Member class would be responsible for setting session variables, etc. Manager just checks user/pass.
    $manager = new AccountsManager ();


?><!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8" />

        <style>

            *
            {
                font-family: "Segoe UI", "Trebuchet MS", Tahoma, Arial, Helvetica, sans-serif;
            }

            body
            {
                margin: 4em 6em;
                line-height: 1.6em;
                font-size: smaller;
            }

            header
            {
                border-bottom: 2px solid #efefef;
                margin-bottom: 3em;
                padding-bottom: 1em;
            }

            h1, h2, h3, h4, h5, h6
            {
                font-weight: normal;
                letter-spacing: 1px;
                color: royalblue;
            }

            h5, h6
            {
                font-weight: bold;
            }

            header h1 sub, header h1 sup
            {
                font-size: small;
                color: #FF4400;
                letter-spacing: 2px;
            }

            section
            {
                border-bottom: 1px dotted #ccc;
                padding-bottom: 2em;
                margin-bottom: 3em;
            }

            table
            {
                border: 1px solid #eee;
                padding: 1em;
                border-right-width: 2px;
                border-bottom-width: 2px;
            }

            th
            {
                text-align: left;
                font-variant: small-caps;
                border-bottom: 1px dotted #ccc;
                padding-bottom: .75em;
                margin-bottom: .75em;
                letter-spacing: 1px;
                color: #FF4400;
            }

            td:hover
            {
                background-color: skyblue;
            }

            td
            {
                margin: 0;
                display: table-cell;
                padding: .5em;
            }

            pre
            {
                font-family: "Droid Sans Mono", Consolas, "Courier New", Courier, monospaced;
                border: 1px solid #E4E4E4;
                padding: 1em;
                line-height: 1em;
            }

            .error
            {
                color: red;
                border: 1px dotted #ccc;
            }

            .success
            {
                color: forestgreen;
                border: 1px dotted #e0e0e0;
            }

            .error, .success
            {
                padding: .75em;
                background-color: #FFFFCC;
                border: 1px solid #E4E4E4;
            }

        </style>

        <title>Sample Login System - Test Exceptions</title>
    </head>

    <body>

        <header>
            <h1>Simple Login System <sup>demonstrating exceptions&hellip;</sup></h1>
        </header>



        <section>
            <h2>No database required</h2>

            <p>To avoid time setting up your environment, this test simply uses a class that stores an array of accounts.
            Obviously, this isn't persistent (at this time) and it doesn't actually save anything anywhere except in the
            array during the script's lifetime. Upon the next request, the previous accounts will be erased.</p>
        </section>



        <section>
            <h2>Creating accounts...</h2>

            <?php

                $createList =
                    array (

                        array (
                            'username' => 'Daniel Elkins',
                            'password' => 'delkins[not-pass-for-anything]',
                        ),

                        array (
                            'username' => 'Jennifer Lynn',
                            'password' => 'lilJenn',
                        ),

                        array (
                            'username'=> 'Charlie Dog',
                            'password'=> 'grrrrr',
                        ),

                    );

                if ( $manager->setAllAccounts ( $createList ) instanceof AccountsManager ) : ?>

                    <p><strong>Accounts were created successfully!</strong> They should be listed in
                    a table below.</p>

                <?php

                else :

                ?>

                    <p class="error">There was an error creating your accounts...</p>

                <?php

                endif;

            ?>

        </section>


        <section>
            <h2>List of accounts</h2>

            <?php echo $manager; ?>

        </section>


        <section>
            <h2>Trying to create one that already exists...</h2>

            <?php

            try
            {
                $manager->AddAccount ( 'Daniel Elkins', 'delkins[not-pass-for-anything]'); ?>

                <p class="success">Account created successfully!</p>

                <?php

            }
            catch ( AccountsInsertException $exception )
            {
                ?>

                <p class="error"><?= $exception->getMessage (); ?></p>

                <?php

            }

            ?>

        </section>


        <section>
            <h2>Showing accounts again</h2>

            <?php echo $manager; ?>

        </section>


        <section>
            <h2>Valid login test</h2>

            <p>Logging in user `Daniel Elkins`&hellip;</p>

            <?php

            if ( $manager->Authenticate ( new Member ( 'Daniel Elkins', 'delkins[not-pass-for-anything]' ) ) ) : ?>

                <p class="success">Authentication successful!</p>

                <?php

            else :

            ?>

                <p class="error">Unable to login; invalid username or password!</p>

                <?php

            endif;

            ?>

        </section>


        <section>
            <h2><strong>Invalid</strong> login test</h2>

            <p>Logging in user `Doesnt_Exist`&hellip;</p>

            <?php

            if ( $manager->Authenticate ( new Member ( 'Doesnt_Exist', '1234' ) ) ) : ?>

                <p class="success">Authentication successful!</p>

                <?php

            else :

            ?>

                <p class="error">Unable to login; invalid username or password!</p>

                <?php

            endif;

            ?>

        </section>


        <section>
            <h2>Debug information</h2>

            <?php $manager->__Debug (); ?>

        </section>

    </body>

</html>
\$\endgroup\$
1
  • \$\begingroup\$ When a user is unable to create an account because it already exists, you shouldn't throw an exception. I assume you know what you need to do in such scenario which would most likely be just to re-present the form with the error message. Exceptions are meant for when an object reaches a state where it does not know how to deal with it any further. Unless you intended to not let your object know what to do in such scenario, I think then an exception would be fine. \$\endgroup\$ Commented Jun 25, 2014 at 5:55

1 Answer 1

1
\$\begingroup\$

Thought I see many questions to different pieces of the code, I'll answer to the question about exceptions only.

I believe this is not good use of exceptions (one exception, actually). You use it just to indicate a problem in a function that can return a special value. This is not "exceptional" situation, but rather normal program flow.

Consider constructor - what can you do if you can not create an object? Exception is the only choice (lets do not discuss general question about quality of class design where you can not avoid exceptions from ctor).

Or, if a function sees a problem that it can not handle alone - memory allocation fault, for example (not the best example, probably).

Exceptions are verfy inefficient and complicated way to transfer messages.

\$\endgroup\$
3
  • \$\begingroup\$ I suppose I could return different values. ex: '1' for for 'Account Already Exists', '2' for 'Invalid username', etc. But then where would those error messages be defined? In the script using the class? Should they be constants within the class and check return value with those: if ( $account->Login () == $account::INVALID_USERNAME ) echo 'Invalid username!'; Something like that? Can you provide an example of how you would do it? I've seen so many posts on StackOverflow saying in OOP, exceptions should be used for things like this, I'm confused. \$\endgroup\$ Commented Apr 18, 2012 at 3:56
  • \$\begingroup\$ This is really complicated issue. Exceptions use depends on overall system desing mainly. It is hard to give ultimate answer. In a simple case like your have presented here exceptions are overkill. Constants should be defined as class constants: const CONST_NAME = const_value; and then you can access them as static properties: ClassName::CONST_NAME Typical OOP system uses several define at the very beginning of execution to registed autoload function, etc. After that - class constants only. \$\endgroup\$
    – Michael
    Commented Apr 18, 2012 at 7:52
  • \$\begingroup\$ +1 for "Save exceptions for exceptional events". I always felt that exceptions get overused, especially in PHP code (probably because they were a relatively recent addition to the language and programmers love shiny new things). \$\endgroup\$
    – GordonM
    Commented Jul 16, 2012 at 10:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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