3
\$\begingroup\$

I am new to the world of coding as well as XHTML and PHP. As a way to gain more experience as well as to put theory into practice including a way to challenge myself, I put together the following code that calculates values a user enters into a form.

This was to test my knowledge regarding PHP's own functions but also creating custom functions. I am sure the code can be improved and would appreciate any advise.

The one thing I was unable to do was to only display the result and to hide the form once it has been posted preferably on the same page i.e. not redirecting the user to another page.

I have also tried to keep business logic and presentation as much as possible.

Appreciate any advise as well as constructive criticism.

Main Page Code - Calculator.php

<?php

require_once('includes/base.php');
require_once('functions.php');

?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

<html>
<head>
    <title>Calculator</title>
</head>
<body>
    <h1>Calculator Instructions</h1>
    <p>Below are instructions on using the calculator</p>
    <ol>
        <li>You must specify 2 values in the fields below</li>
        <li>You must select an arithmetic operator which includes the list below
            <ul>
                <li>Multiply</li>
                <li>Divide</li>
                <li>Modulus</li>
                <li>Add</li>
                <li>Subtract</li>
            </ul>
        </li>
        <li>Simply submit the form once you have specified 2 values and selected arithmetic operator</li>
    </ol>

    <h2>Calculator</h2>

    <div>

    <?php

        if(isset($_POST['submit'])) {
            echo mycalculator();
        }

    ?>

    </div>


    <form name="calculator" method="post" action="calculator.php">
        <div>
            Field 1: <br />
            <input type="input" name="field1" value="<?php if(isset($_POST['field1'])) { echo $_POST['field1']; } ?>" />
        </div>

        <div>
            Field 2: <br />
            <input type="input" name="field2" value="<?php if(isset($_POST['field2'])) { echo $_POST['field2']; } ?>" />
        </div>

        <div>

            Select an arithmetic operator: <br />
            <select name="field3">
                <option value="">Please select an option</option>

                <?php

                echo myselect();

                ?>

            </select>
        </div>

        <div>

        </div>

        <div>
            <input type="submit" name="submit" value="submit" />
            <input type="reset" name="reset" value="reset" />
        </div>
    </form>
</body>
</html>

Define and Set directory and path - Base.php

<?php

//Retrieve parent directory
$parentdirectory = dirname(dirname(__FILE__));
$unixpath = str_replace('\\', '/', $parentdirectory);

//Define root directory
define('BASE_DIRECTORY', $parentdirectory);

//Set includes directory
ini_set('include_path', $parentdirectory.'\includes');

?>

Business Logic - Functions.php

<?php

//Generate arithmetic menu
function myselect () {

    //Define arithmetic operators
    $selectarray = array('multiply', 'divide', 'modulus', 'add', 'subtract');

    $status = '';

    foreach($selectarray as $seletkey => $selectvalue) {

        if($_POST['field3'] == $selectvalue) {

            $status = 'selected';
        }

        else {

            $status = '';
        }

        echo '<option value="'.$selectvalue.'" '.$status.'>'.$selectvalue.'</option>';
    }
}


//Validate if form has been submitted
if(isset($_POST['submit'])) {

    //Generate errors if form is not completed
    function mycalculator() {

    //Instatiate form fields, error messages and arithmetic operators
    $formfields = array('a' => 'field1', 'b' => 'field2', 'c' => 'field3');
    // $formfieldkeys = array_values($formfields);
    $formfieldkeys = array_combine(range(1, count($formfields)), array_values($formfields));

    //Instantiate error messages array
    $errormsg = array();

    //Validate if form field has been set
        foreach($formfieldkeys as $formfieldkey => $formfieldvalue) {

            $formfield = isset($_POST[$formfieldvalue]) ? $_POST[$formfieldvalue] : '';

                //Return error message
                if(empty($formfield)) {

                    $errormsg = 'Please specify a value for field '.$formfieldkey.'<br />';

                    /* Return error message for the specific field concerned using array_values
                    $errormsg = 'Please specify a value for field '.($formfieldkey + 1).'<br />';
                    */

                    echo $errormsg;
                }

                //Validate if the values being posted for fields 1 - 2 are numeric
                if(in_array($formfieldvalue, array('field1','field2'))) {
                    if(!is_numeric($formfield)) {

                        $errormsg = 'You must provide a numeric value for field '.$formfieldkey.'<br />';

                        /* Return error message for the specific field concerned using array_values
                        $errormsg = 'You must provide a numeric value for field '.($formfieldkey + 1).'<br />';
                        */

                        echo $errormsg;
                    }
                }

                if(!empty($formfield)) {

                //Extract individual array values
                // extract($formfields);

                // $field1 = $_POST[$a];
                // $field2 = $_POST[$b];
                // $field3 = $_POST[$c];

                //Dynamic Variables
                ${"field{$formfieldkey}"} = $_POST[$formfieldvalue];

                    if(!empty($field1) && !empty($field2) && !empty($field3)) {

                        if($field3 == 'multiply') {
                            $calculate = $field1 * $field2;
                            // echo $calculate;
                        }

                        elseif($field3 == 'divide') {
                            $calculate = $field1 / $field2;
                            // echo $calculate;
                        }

                        elseif($field3 == 'modulus') {
                            $calculate = $field1 % $field2;
                            // echo $calculate;
                        }

                        elseif($field3 == 'add') {
                            $calculate = $field1 + $field2;
                            // echo $calculate;
                        }

                        elseif($field3 == 'subtract') {
                            $calculate = $field1 - $field2;
                            // echo $calculate;
                        }

                        else {
                            $calculate = 'error';
                        }

                        if($calculate > 0) {

                        return 'The answer is '.$calculate; 

                        }

                    }

                }
        }
    }

//Debug
// print_r($_POST);

}
?>
\$\endgroup\$
2
\$\begingroup\$

This is going to have a lot of extensibility concerns. What if I want to add exponentials or scientific notation? What if I want to chain operations? What if I want to ensure someone doesn't put unsafe code that could bork my webserver in the POST variable?

https://stackoverflow.com/questions/2401706/where-to-sanitize-php-post-input

All that said, I'm impressed by how good a job you did of separating your presentation from your business logic. I've seen a lot uglier PHP. The main page code is nicely mainly HTML, and the business logic is separated out.

From here, POA:

  1. Refactor for an Object Oriented Approach
  2. Sanitize User input
  3. Consider breaking apart pieces of the UI into separate chunks that are added dynamically, which reduced the maintenance burden. For example, you may want to put

        Select an arithmetic operator: <br />
        <select name="field3">
            <option value="">Please select an option</option>
    
            <?php
    
            echo myselect();
    
            ?>
    
        </select>
    </div>
    

In an operations.php that you include. This allows for easy modification/adding of stuff later.

Also, you'll probably want to look into AJAX.

\$\endgroup\$
3
  • \$\begingroup\$ Software Solution - Thanks for your comments and feedback. I didn't look at extensibility of the code at this stage as I was keen to learn and come to grasps with functions which I must say is still very new to me. Nevertheless, I'll certainly bear in mind about the scalability of my code. What are chain commands? Also I didn't look at sanitizing the data apart from a very basic check of whether the value was numeric. \$\endgroup\$ Apr 26 '11 at 8:37
  • \$\begingroup\$ Software Solution - continued -I assume POA refers to Points of Action. I have no idea how object oriented programming works at this stage but am hoping to touch on it soon. I'll remember about sanitizing user data. How else would you suggest I separate the UI into separate blocks/chunks of code? From a coding perspective, was my code easy to follow? How would you rate it, 1 being absolute crap and 10 being the best of the breed? Where did I make mistakes? \$\endgroup\$ Apr 26 '11 at 8:41
  • \$\begingroup\$ I'd go further and suggest that no html at all should be in your 'business logic'. If your dev team is split front end & server side, then you need to ensure any html is clearly presented and accessible to the other developers. Doing a quick loop and using helper functions such as set_select() in CodeIgniter should reduce the visible server side logic. \$\endgroup\$ May 26 '11 at 7:59
1
\$\begingroup\$

I'm not sure about the function names myselect and mycalculator. They aren't good function names as it stands, but are they just example names that you're planning to change later? Putting the 'my' prefix on a function isn't very useful, since all functions belong to somebody. Often the 'my' prefix is used in example code in books and tutorials to emphasise that this is code written by the user, but it's not good practice in the real world.

Good function names should:

  • As far as possible, tell you exactly what the function does. This might include things like whether the generated HTML is printed directly or returned to the caller, which is often differentiated as print_foobar and render_foobar.
  • Not mention any details of the function that are irrelevant to the caller: who wrote the function, whether it's implemented in PHP or C++, etc.
  • Take into account the context in which the function will be called. An object method often needs less context in the name than a free function, because the name of the object will give some context. If you had a free function for rendering a calculator you might call it render_calculator(), but the equivalent method on a Calculator class would probably just need to be render, since you'll typically call it like $calc->render().
  • Follow any (explicit or implicit) naming conventions in (by order of preference) the current class, the current code module, the major libraries you're working with, and the prevailing language style as a whole. PHP is famously bad at adhering to consistent conventions in its standard library functions, but if you're using (say) Zend Framework, you should consider following their conventions.

You will probably want to give your POST variables better names, similar to the way you'd name variables within ordinary code. field3 is a bad name, operator might be better. For field1 and field2 you might be better to emphasise that they are left and right-hand sides of an expression: expr_lhs and expr_rhs might work better.

You want to avoid referring to the $_POST array in more than one place, because it implicitly couples together the various bits of code that refer to it. If you change the keys of your POST data, you don't want to have to update lots of different bits of the code (even worse, you don't want to have trouble tracing where it's referred to, possibly across lots of different files and directories, and fail to update one place). PHP makes it easy to refer to $_POST all over the place, but this doesn't mean it's good practice.

A better option might be to have one piece of code that's responsible for unpacking the $_POST` array and packing it into objects that the rest of the code can use. For example, you could have an expression class:

<?php
class Expression {
   public $lhs;
   public $rhs;
   public $operator;
}
?>

And have a function that unpacks $_POST and returns an instance of this.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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