How much is too much code?

[DoubleDee]

My Log-In and Change-Password scripts are getting a bit unwieldy coming at 300+ lines of code each.

I am using traditional Procedural code.

I know a lot of OOP developers would say that if you have more than 30 lines of code in a class it is too much.

What do you think?

My Log-In script has a couple of calls to MySQL and a couple nested IF-THEN-ELSE's...


Debbie

[Jake Arkinstall]

People who program OOP in PHP are more likely to:

• have come from a background of other programming languages
  -or-
• are more likely to explore other programming languages with more rigour

This is because many other programming languages have OOP functionality and therefore share certain concepts that people who write OO in other programming languages are already used to.

However, many languages have certain restrictions. Java, for example (which is also C-based), only allows one class per file, where the class is the same name as the file (apart from the .java obviously). After a good period of time programming in Java, you learn to appreciate the benefits of that (autoloading is much easier, organisation and debugging, again, easier).

Some practises are overall pointless habits. I like to keep my classes concise so that further functionality is made with extending those classes and making an overall more flexible system. However, 30 lines is a ridiculous limit. If a class is under 20 lines, without being an extension of another class (with those certain lines setting variables which affect the parent class behaviour), then it seems rather limited. In fact it sounds like a data-holder (such as a record or struct), which is fair enough for that case.

300 lines of code for a log in sounds like a lot. However, taking into account HTML output, form validation, session handling, database connectivity etc then Im not entirely surprised with procedural code. In OOP it takes less code because you've generally already pre-written validation code for the field-modifying methods, form-data accessing methods, you'd already have the database connection open and the update queries would already be pre-written. All you'd have left to do for the actual pages is write the form code (unless you've made a mechanism to automate that) and write a method to control what happens when the form's submitted... unless you've already made an automation for that.

I've built frameworks when forms can be automatically generated from models, and the handling and validation is built in. Complete waste of time, I spent more time customising the automatic functionality than I would have done making them from scratch.

[DoubleDee]

Jake,

Interesting response, but I'm not sure what your advice to me is?!

Maybe it would help to see my code?

Here is my log_in.php...

<?php //Build Date: 2012-02-04

    // Initialize Session.
    session_start();

    // Initialize Variables.
    $_SESSION['loggedIn'] = FALSE;
    $salt = '';

    // Access Constants.
    require_once('../config/config.inc.php');


    // *************************************************************
    // HANDLE FORM.                                                                                                 *
    // *************************************************************
    if ($_SERVER['REQUEST_METHOD']=='POST'){
        // Form was Submitted (Post).

        // Initialize Errors Array.
        $errors = array();

        // Trim all form data.
        $trimmed = array_map('trim', $_POST);


        // ************************
        // Validate Form Data.        *
        // ************************

        // Check Email.
        if (empty($trimmed['email'])){
            $errors['email'] = 'Please enter your E-mail address.';
        }else{
            $email = $trimmed['email'];
        }

        // Check Password.
        if (empty($trimmed['pass'])){
            $errors['pass'] = 'Please enter your Password.';
        }else{
            $pass = $trimmed['pass'];
        }


        // ********************************
        // Check for Activation & Salt.        *
        // ********************************
        if (empty($errors)){
            // Form Complete.

            // **********************
            // Find Member Record.    *
            // **********************

            // Connect to the database.
            require_once(WEB_ROOT . 'private/mysqli_connect.php');

            // Build query.
            $q1 = 'SELECT activation_code, salt
                            FROM member
                            WHERE email=?';

            // Prepare statement.
            $stmt1 = mysqli_prepare($dbc, $q1);

            // Bind variable to query.
            mysqli_stmt_bind_param($stmt1, 's', $email);

            // Execute query.
            mysqli_stmt_execute($stmt1);

            // Store results.
            mysqli_stmt_store_result($stmt1);

            // Check # of Records Returned.
            if (mysqli_stmt_num_rows($stmt1)==1){
                // Member Email found.

                // Bind result-set to variables.
                mysqli_stmt_bind_result($stmt1, $activationCode, $salt);

                // Fetch record.
                mysqli_stmt_fetch($stmt1);


                // **********************
                // Verify Activation.        *
                // **********************
                if (is_null($activationCode)){
                    // Account Activated.

                    // ******************
                    // Check for Salt.    *
                    // ******************
                    if (is_null($salt) || strlen($salt)==0){
                        // No Salt Found.
                        $errors['pass'] = 'A fatal error has occurred.  Contact the System Admin';
                    }
                }else{
                    // Account Not Activated.
                    $errors['pass'] = 'Please activate your Account before logging in.';
                }// End of VERIFY ACTIVATION

                // Close prepared statement.
                mysqli_stmt_close($stmt1);
            }// End of FIND MEMBER RECORD
        }//End of CHECK FOR ACTIVATION & SALT


        // ****************************
        // Attempt to Log-In Member.    *
        // ****************************
        if (empty($errors)){
            // Valid Member Record.

            // ************************
            // Create Password Hash.    *
            // ************************
            $hash = hash_hmac('sha512', $pass . $salt, VINEGAR);


            // ************************
            // Find Activated Member.    *
            // ************************

            // Build query.
            $q2 = 'SELECT id, first_name
                            FROM member
                            WHERE email=? AND hash=?';

            // Prepare statement.
            $stmt2 = mysqli_prepare($dbc, $q2);

            // Bind variables to query.
            mysqli_stmt_bind_param($stmt2, 'ss', $email, $hash);

            // Execute query.
            mysqli_stmt_execute($stmt2);

            // Store results.
            mysqli_stmt_store_result($stmt2);

            // Check # of Records Returned.
            if (mysqli_stmt_num_rows($stmt2)==1){
                // Member was Found.

                // Bind result-set to variables.
                mysqli_stmt_bind_result($stmt2, $memberID, $memberFirstName);

                // Fetch record.
                mysqli_stmt_fetch($stmt2);

                // Set Session variables.
                $_SESSION['memberID'] = $memberID;
                $_SESSION['memberFirstName'] = $memberFirstName;
                $_SESSION['loggedIn'] = TRUE;

                // Close prepared statement.
                mysqli_stmt_close($stmt2);

                // Close the connection.
                mysqli_close($dbc);


                // ********************
                // Redirect Member.        *
                // ********************

                // Add-a-Comment Redirect.
                if (isset($_GET['addComment']) && ($_GET['addComment']==TRUE)){
                    header("Location: " . BASE_URL . "articles/add_comment.php");

                    // End script.
                    exit();
                }

                // Normal Redirect.
                if (isset($_SESSION['returnToPage'])){
                    header("Location: " . BASE_URL . $_SESSION['returnToPage']);
                }else{
                    // Take user to Home Page.
                    header("Location: " . BASE_URL . "index.php");
                }

            }else{
                // Member Not Found.
                $_SESSION['loggedIn'] = FALSE;
                $errors['pass'] = 'The E-mail and Password do not match those on file.';
            }// End of FIND ACTIVATED MEMBER

        }else{
            // Invalid Member Record.
            // Drop through to display Form.
        }//End of ATTEMPT TO LOG-IN MEMBER

    }else{
        // Form was not Submitted (Get).
        // Drop through to display Form.
    }//End of HANDLE FORM
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
    "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">

<head>
    <!-- ################## DEBBIE ##################### -->
    <!-- HTML Metadata -->
    <title>Member Log-In</title>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <meta name="description" content="" />
    <meta name="keywords" content="" />

    <!-- Page Stylesheets -->
    <link type="text/css" rel="stylesheet" href="/css/_main.css" />
    <link type="text/css" rel="stylesheet" href="/css/_layout.css" />
    <link type="text/css" rel="stylesheet" href="/css/top_menu.css" />
    <link type="text/css" rel="stylesheet" href="/css/components.css" />
</head>

<body>
    <div id="pageWrapper" class="clearfix">
        <div id="pageInner">
            <!-- BODY HEADER -->
            <?php    require_once(WEB_ROOT . 'components/body_header.inc.php');    ?>

            <!-- MIDDLE COLUMN -->
            <div id="pageMidCol_1">

                <!-- LOG-IN FORM -->
                <form id="login" action="" method="post">
                    <fieldset>
                        <legend>Log-In</legend>
                        <ul>
                            <!-- Cookies Note -->
                            <li id="acceptCookies"><b>*</b>Your browser must accept cookies in order to log in.</li>

                            <!-- Article Heading -->
                            <?php
                                if (isset($_GET['addComment']) && ($_GET['addComment']==TRUE)){
                                    // Trying to Add Comment.
                                    if (isset($_SESSION['heading'])){
                                        // Article Heading exists.
                                        echo '<li>
                                                        Please log-in to comment on the article:<br />
                                                        <span id="articleHeading">"' . $_SESSION['heading'] . '"</span>
                                                    </li>';
                                    }
                                }//End of CHECK FOR ARTICLE HEADING
                            ?>

                            <!-- Email -->
                            <li>
                                <label for="email">E-mail:</label>
                                <input id="email" name="email" type="text" maxlength="60" />
                                <?php
                                    if (!empty($errors['email'])){
                                        echo '<span class="error">' . $errors['email'] . '</span>';
                                    }
                                ?>
                            </li>

                            <!-- Password -->
                            <li>
                                <label for="pass">Password:</label>
                                <input id="pass" name="pass" type="password" maxlength="40" />
                                <?php
                                    if (!empty($errors['pass'])){
                                        echo '<span class="error">' . $errors['pass'] . '</span>';
                                    }
                                ?>
                            </li>

                            <!-- Submit Form -->
                            <li>
                                <input type="submit" name="logIn" class="button" value="Log In" />
                            </li>
                        </ul>
                    </fieldset>
                </form>

            </div><!-- End of #MIDDLE -->

        </div><!-- End of #INNER -->
    </div><!-- End of #WRAPPER -->

    <!-- BODY FOOTER -->
    <?php    require_once(WEB_ROOT . 'components/body_footer.inc.php');    ?>
</body>
</html>

In the next couple of days I hope to have a working demo of my entire website up on the Internet so people trying to help me out can actually see what a user would see.

I think the code above is pretty "tight", but it can be pretty intense reading it with all of its moving parts and branches.

Sometimes it seems like it would be better if it was broken up into 5-6 pieces.

Conceptually what I am doing in my code is this...

1.) Verify Form was completed
2.) Verify Member Account was Activated
3.) Verify Member Record has a Salt
4.) Create Hash based on Form Inputs and Salt
5.) Attempt to Log In Member (i.e. Find Member Record)
6.) Redirect Member


Debbie

[ahundiak]

Overall your code looks fine. Considerably better than a lot of code that is out there.

It can of course be broken up to ease the maintainability and to perhaps allow some reuse and easier testing.

Take everything from <DOCTYPE and down and move it to login.template.php. That cuts your file sizes in half and reduces the coupling between processing and presentation.

Next, replace if ($_SERVER['REQUEST_METHOD']=='POST'){ ... } with if ($_SERVER['REQUEST_METHOD']=='POST'){ $errors = process_posted_login_form($POST); }

You will quickly notice that when your new process_posted_login_form function encounters an error, all it needs to do is to return the error. That in turn means that much of your indenting can go away.

Lots more that could be done but best to get your site working then worry about refactoring.

This is also well worth a read even if you have no intentions of ever using Symfony 2:
http://symfony.com/doc/current/book/..._symfony2.html

[Cups]

Drop the comments.

Lines like this add nothing - the code functions names are documentation themselves.

                    // End script. 
                    exit(); 

Only comment things that need it, when you deviate from the norm for any reason.

Get hold of a copy of "Clean Code" by Bob Martin and learn and understand why you need to liberate yourself from this tyranny.

[DoubleDee]

Overall your code looks fine. Considerably better than a lot of code that is out there.

It can of course be broken up to ease the maintainability and to perhaps allow some reuse and easier testing.

Take everything from <DOCTYPE and down and move it to login.template.php. That cuts your file sizes in half and reduces the coupling between processing and presentation.

Next, replace if ($_SERVER['REQUEST_METHOD']=='POST'){ ... } with if ($_SERVER['REQUEST_METHOD']=='POST'){ $errors = process_posted_login_form($POST); }

You will quickly notice that when your new process_posted_login_form function encounters an error, all it needs to do is to return the error. That in turn means that much of your indenting can go away.

Lots more that could be done but best to get your site working then worry about refactoring.

This is also well worth a read even if you have no intentions of ever using Symfony 2:
http://symfony.com/doc/current/book/..._symfony2.html

So, for now, you think my code is good enough, as-is?

(I'm torn between refactoring things to death and getting my site up and working. "Analysis-paralysis" is my specialty!!)


Debbie

[ahundiak]

So, for now, you think my code is good enough, as-is?

(I'm torn between refactoring things to death and getting my site up and working. "Analysis-paralysis" is my specialty!!)

Debbie

Your code is fine as is. Get your site working then revisit.

[DoubleDee]

Your code is fine as is. Get your site working then revisit.

Thanks for the kind words!

Maybe sometime soon you can give me some tips on how to switch to a "Templating System"?

Thanks,


Debbie

[brense]

Well... If you can find what you are looking for in 300 lines of code then you can use 300 lines of code in your class But I think that for most people 300 lines of code in a class are way to much (unless they are devided over 100 functions :P) It would be impossible to maintain your code. And 300 lines of code are much more likely to produce bugs then 30 lines.

Only one thing is really important though. Each function in your class should only be responsible for 1 thing. So if you have a createUser function that functions should only have code needed to create a user, it should not have code for sending emails or sanitize user input. You should create seperate functions for that. This makes your code easier to maintain and re-usable which means that in the end it saves you months of works.

Personally I prefer to spend a lot of time thinking about the architecture of my application and only a small amount of time goes into actual programming. Once I have everything figured out on paper its peanuts to transform it into actual code. But yes, sometimes it means I never get to the actual programming part