0

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
    {

        public function __construct ( $message = null, $code = 0, Exception $previous = null )
        {
            parent::__construct ( $message, $code, $previous );
        }

    }




    class AccountsInsertException extends Exception
    {

        public function __construct ( $message = null, $code = 0, Exception $previous = null )
        {
            parent::__construct ( $message, $code, $previous );
        }

    }




    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>
  • 1
    Sounds like it's a better question for http://codereview.stackexchange.com. Also, you don't need to repeat the constructor declaration if all you're doing is call the exact same method in the `parent::`. – deceze Apr 16 '12 at 06:06
  • Wow, I had no idea that even existed. I will post it over there, thanks. – Daniel Elkins Apr 16 '12 at 06:31

1 Answers1

1

Is this the correct way to use exceptions and handle them on the caller side?

Seems like a reasonable approach to me. You are throwing exceptions specific to the class, thus it's easy to catch or propagate them, and only the specific ones instead of having to catch everything and filter.

Should I even be using exceptions for this?

If you consider it an exceptional circumstance that an account exists, yes.

Are my skeleton custom exceptions correct?

Yes, although you could consider adding metadata into them, such as the name of the account which was being created into AccountInsertException. It may not be necessary, but if you find yourself in a situation where it would be useful, it's just something to consider.

Otherwise the code is kind of a mess in places though but I'm assuming it's partially because of the example.

Jani Hartikainen
  • 42,745
  • 10
  • 68
  • 86
  • What do I do if it's not "exceptional circumstances"? trigger_error ()? How does my class "report back" to the main application that there was a problem then? I'd rather not have to use Dependency Injection and pass a error handler object into the constructor of all of my classes... try/catch seems more elegant and simple. Also, the code was "thrown together" and there are obvious cases where I could have used PHP's built-in functions instead, but what exactly is "a mess"? I really like your suggestion about modifying my exception class to show account details, etc. Thanks. – Daniel Elkins Apr 16 '12 at 06:21
  • I would avoid trigger_error as it's impossible for the calling code to handle it in a sane fashion. You would typically use return values. For example, in a method which queries the DB and returns something, you would typically return `null` in a case where nothing is found instead of throwing. Mess: Building HTML inside the funcs, the classes don't follow the recommended naming/coding styles, stuff like that. I probably made it sound worse than it is ;) – Jani Hartikainen Apr 16 '12 at 07:23
  • It was an example thrown together just for this very question. I don't see anything wrong with returning a barebones HTML table in a __toString () method anyway; that's what should be expected as output for tabular data (in my opinion). If it were anything but __toString () I wouldn't do that. As far as naming conventions, which "recommended ones" are you referring to? Zend? PEAR? There's so many and there is no "correct" one. But yeah, the code was tossed together just for an example, not sure why that would be critiqued at all. Not bothered by it in the least, it just figures I guess. – Daniel Elkins Apr 16 '12 at 08:58
  • Yeah just wanted to mention it in case there was something to improve about it :) – Jani Hartikainen Apr 16 '12 at 10:11