4
\$\begingroup\$

I am willing to switch to OOP concept, but before working on that I would like to know whether what I have working so far is OK, or if any major change is required?

For every form I create 3 files:

  1. for front end html
  2. a js file as a mediator between client and server
  3. a server side php file which handles data services.

Please help me improve my approach.

<!--
======================================================================================
First File for front end HTML   : drivers.php
======================================================================================
-->

    <script src=  "js/drivers.js"></script>


    <label class="uk-icon-user uk-article-title formHeading vs_form_icon"></label>
    <label class="formHeading">Drivers</label><br><br>

    <form class = "uk-form uk-form-horizontal">

        <div class = "uk-form-row">
            <label class = "uk-form-label" for = "drivers_name">Name</label>
            <div class = "uk-form-controls">
                <input type = "text" id = 'drivers_name' tabindex ="1301" class = 'uk-form-small autocomplete_with_new_value_driver PascalCase SelectOnFocus width400'>
            </div>
        </div>

        <div class = "uk-form-row">
            <label class = "uk-form-label" for = "drivers_rename">Re-Name</label>
            <div class = "uk-form-controls">
                <input type = "text" id = 'drivers_rename' tabindex ="1302" class = 'uk-form-small PascalCase SelectOnFocus width400'>
            </div>
        </div>

        <div class = "uk-form-row">
            <label class = "uk-form-label" for = "drivers_pres_address">Present Address</label>
            <div class = "uk-form-controls">
                <textarea id = 'drivers_address' tabindex ="1303" class = 'height100 width400 SelectOnFocus'> </textarea>
            </div>
        </div>

        <div class = "uk-form-row">
            <label class = "uk-form-label" for = "drivers_perm_address">Perm. Address</label>
            <div class = "uk-form-controls">
                <textarea id = 'drivers_perm_address' tabindex ="1304" class = 'height100 width400 SelectOnFocus'> </textarea>
            </div>
        </div>

        <div class = "uk-form-row">
            <label class = "uk-form-label" for = "drivers_engine_no">Licence No.</label>
            <div class = "uk-form-controls">
                <input type = "text" id = 'drivers_licence_no' maxlength="30" tabindex = "1305" class = 'uk-form-small width400 SelectOnFocus'>
            </div>
        </div>

        <div class = "uk-form-row">
            <label class = "uk-form-label" for = "drivers_engine_no">Licence Valid Upto</label>
            <div class = "uk-form-controls">
                <input type="text" id='drivers_licence_valid_upto' tabindex ="1306" class ='uk-form-small  datePicker width100'>
            </div>
        </div>

        <br>
        <div id="drivers_buttons_group" align="center" class="buttonsGroup">
            <button id='drivers_btnSaveMaster' tabindex ="1351" class="uk-button uk-button-primary width100" type="button">Save</button>
            <button id='drivers_btnReset' tabindex ="1352" class="uk-button uk-button-primary width100" type="reset">Reset</button>
            <button id='drivers_btnDeleteEntry' tabindex ="1353" class="uk-button uk-button-primary width100" type="button">Delete</button>
        </div>

    </form>
// ===============================================================
// Second .JS File : drivers.js
// ===============================================================
    $(document).ready(function() {
    //------------------------------------------------------------------------------ Date Picker
        $(function() {
            $(".datePicker").datepicker({dateFormat: 'dd-mm-yy'});
        });

        //-------------------------------------------------------------------------- Plugin For Combobox with new values
        var $input = $(".autocomplete_with_new_value_driver").autocomplete({
            source: "php/driverslist.php?nameonly=Y",
            minLength: 0
        }).addClass("no-border-right ");


        $("<button type='button'>&nbsp;</button>")
                .attr("tabIndex", -1)
                .attr("title", "Show All Items")
                .insertAfter($input)
                .button({
                    icons: {
                        primary: "ui-icon-triangle-1-s"
                    },
                    text: false
                })
                .removeClass("ui-corner-all")
                .addClass("uk-button border-1-px combobox_with_new_value_button")

    //            .removeClass()
    //            .addClass("combobox_button ui-corner-right ui-button-icon")
                .click(function() {
                    // close if already visible
                    if ($input.autocomplete("widget").is(":visible")) {
                        $input.autocomplete("close");
                        return;
                    }
                    $(this).blur();
                    $input.autocomplete("search", "");
                    $input.focus();
                });

    //------------------------------------------------------------------------------ Name Lost Focus.....
        $("#drivers_name").blur(function() {
            if ($.trim($("#drivers_name").val()) === '') {
                return;
            }

            $("#drivers_rename").val($("#drivers_name").val());
            var _name = $("#drivers_name").val();
            $.post("php/drivers_serverside.php",
                    {
                        task: 'Get Record',
                        name: _name
                    }
            ).fail(
                    function(xhr, textStatus, errorThrown) {
                        alert('Error communicating to server. \n' + errorThrown);
                    }
            ).success(
                    function(data)
                    {
                        var result_array = $.parseJSON(data);
                        if (result_array) {
                            $('#drivers_address').val(result_array['address']);
                            $('#drivers_perm_address').val(result_array['perm_address']);
                            $('#drivers_licence_no').val(result_array['licence_no']);
                            if (result_array['licence_valid_upto'] === '') {
                                $('#drivers_licence_valid_upto').val('');
                            } else {
                                $('#drivers_licence_valid_upto').val(convertDateFormat(result_array['licence_valid_upto']));
                            }
                        }
                    }
            );
        });

        //        ------------------------------------------------------------------------------ Save Master button click.....
        $("#drivers_btnSaveMaster").click(function() {
            if (!validform_master()) {
                return;
            }

            $.post("php/drivers_serverside.php",
                    {
                        task: 'Save Master',
                        PK_C_name: $("#drivers_name").val(),
                        FL_C_name: $("#drivers_rename").val(),
                        FL_C_address: $("#drivers_address").val(),
                        FL_C_perm_address: $("#drivers_perm_address").val(),
                        FL_C_licence_no: $("#drivers_licence_no").val(),
                        FL_D_licence_valid_upto: $("#drivers_licence_valid_upto").val(),
                        FL_C_ac_type: 'Driver'
                    }
            ).success(function() {
                drivers_initialise();
            }).fail(
                    function(xhr, textStatus, errorThrown) {
                        alert('Error communicating to server. \n' + errorThrown);
                    }
            );
        });
    //------------------------------------------------------------------------------ Reset button click.....
        $("#drivers_btnReset").click(function() {
            drivers_initialise();
        }
        );
    //------------------------------------------------------------------------------ Delete button click.....
        $("#drivers_btnDeleteEntry").click(function() {
            $sure = "No";
            $(function() {
                $("#dialog-confirm").dialog({
                    resizable: false,
                    height: 140,
                    modal: true,
                    buttons: {
                        "Delete": function() {
                            var _name = $("#drivers_name").val();
                            $.post("php/drivers_serverside.php",
                                    {
                                        task: 'Delete Entry',
                                        name: _name
                                    }
                            ).fail(
                                    function(xhr, textStatus, errorThrown) {
                                        alert('Error communicating to server. \n' + errorThrown);
                                    }
                            ).success(
                                    function(data)
                                    {
                                        // check for errors...
                                        var result_array = $.parseJSON(data);
                                        if (result_array['Error'] !== undefined) {
                                            alert('ERROR : \n' + result_array['Error']);
                                            return;
                                        }
                                        //......................
                                        drivers_initialise();
                                    }
                            );
                            $(this).dialog("close");
                        },
                        Cancel: function() {
                            $sure = 'No';
                            $(this).dialog("close");
                        }
                    }
                });
            });
        });
    //------------------------------------------------------------------------------ Close button click.....
        $("#drivers_btnClose").click(function() {
            $("#frm_drivers_master").remove();
            $("#tabs li a[href='#frm_drivers_master']").remove();
        }
        );
        //        ------------------------------------------------------------------------------ Valid Form.....
        function validform() {
            var all_valid = true;
            if (!is_valid('#drivers_name', 'required')) {
                all_valid = false;
            }
        }

    //        ------------------------------------------------------------------------------ Valid Form (Master).....
        function validform_master() {
            all_valid = true;
            return all_valid;
        }

    //        ------------------------------------------------------------------------------ Is Valid Element.....
        function is_valid(this_element, this_criteria) {
            if (this_criteria === 'required' && ($(this_element).val() == '' || parseInt($(this_element).val()) == 0)) {
                if ($(this_element).is("select")) {
                    $(this_element).next().next().delay(3000).css("background-color", "#FFC9C9!important");
                } else
                {
                    $(this_element).delay(3000).css("background-color", "#FFC9C9!important");
                }
                $(this_element).attr('title', 'This field is required.');
                return false;
            } else {
                if ($(this_element).is("select")) {
                    $(this_element).next().next().delay(3000).css("background-color", "");
                } else {
                    $(this_element).delay(3000).css("background-color", "");
                }
                $(this_element).attr('title', '');
                return true;
            }
        }
        //-------------------------------------------------------------------------- initialise
        function drivers_initialise() {
            $('#drivers_name').val('').trigger("change").focus();
            $("#drivers_rename").val("");
            $("#drivers_address").val("");
            $("#drivers_perm_address").val("");
            $("#drivers_licence_no").val("");
            $("#drivers_licence_valid_upto").val("");

            $("#drivers_name").focus();
        }
        ;

        //------------------------------------------------------------------------------ Add button for combobox
        $(".addButton").button({
            icons: {
                primary: "ui-icon-plus"
            }
        });

        //-------------------------------------------------------------------------- Start-up
        drivers_initialise();

    });
<!--    
==============================================================================
Third one, server side PHP for data handling. : drivers_serverside.php
==============================================================================
-->

    <?php

    include '../common/connect.php';
    require_once('../php/vsoft_php_functions.php');
    $MyFunctions = new MyFunctions();

    require_once('../php/vsoft_error_log.php');
    $log = new log();
    $username = "venkatesh";

    if (isset($_POST['task']) || isset($_GET['task']) || isset($_POST['oper'])) {
        $task = "";
        if (isset($_POST['task'])) {
            $task = $_POST['task'];
            $log->user("\n" . 'POST : ' . $task, $username);
        } elseif (isset($_GET['task'])) {
            $task = $_GET['task'];
            $log->user("\n" . 'GET : ' . $task, $username);
        } elseif (isset($_POST['oper'])) {
            $log->user("\n oper : " . $_POST['oper'], $username);
        }

    //------------------------------------------------------------------------------  Delete record
        if ($task == 'Delete Entry') {
            $name = $_POST['name'];
            $code = 0;

            //Find Code...
            $data = null;
            $code = 0;
            $records = mysql_query("SELECT code FROM `tblmainmaster` where name = '$name' and ac_type = 'Driver'");
            if (!$records) {
                echo '{"Error":"' . mysql_error() . '"}';
                return;
            } else {
                $r = mysql_fetch_assoc($records);
            }
            if (mysql_num_rows($records) == 0) {
                echo '{"Error":"Driver not found in master."}';
                return;
            } else {
                $code = $r['code'];
            }
            //---------------------------
            //Check for any existing transactions for that code...
            $data = null;
            $records = mysql_query("SELECT count(*) as entries FROM `tbljobcard` where driver = $code");
            if (!$records) {
                echo '{"Error":"' . mysql_error() . '"}';
                return;
            }

            $r = mysql_fetch_assoc($records);
            $entries = $r['entries'];

            if ($entries > 0) {
                echo '{"error":"Entries($entries) existing. Unable to delete."}';
                return;
            }
            //---------------------------


            $records = mysql_query("delete from tblmainmaster where code = $code");
            if (!$records) {
                echo '{"Error":"' . mysql_error() . '"}';
                return;
            } else {
                echo '{"Status":"Deleted"}';
                return;
            }

    //------------------------------------------------------------------------------  Save Master record
        } else if ($task == 'Save Master') {
            $name = $_POST['PK_C_name'];
            $field_set = $MyFunctions->ConvertToQuery($_POST);

            $fields = "name";
            $records = mysql_query("select $fields from tblmainmaster where name = '$name' and ac_type = 'Driver'");
            if (!$records) {
                echo '{"Error":"' . mysql_error() . '"}';
                return;
            } else {
                if (mysql_num_rows($records) > 0) {
                    $records_update = mysql_query("update tblmainmaster set " . $field_set . " where name = '$name' and ac_type = 'Driver' ");
                    if (!$records_update) {
                        echo '{"Error":"' . mysql_error() . '"}';
                        return;
                    } else {
                        echo '{"Status":"Updated"}';
                        return;
                    }
                } else {
                    $records_update = mysql_query("insert into tblmainmaster set " . $field_set);
                    if (!$records_update) {
                        echo '{"Error":"' . mysql_error() . '"}';
                        return;
                    } else {
                        echo '{"Status":"Added"}';
                        return;
                    }
                }
            }
    //------------------------------------------------------------------------------  Get Record
        } else if ($task == 'Get Record') {
            include '../common/connect.php';

            if (isset($_POST['name'])) {
                $name = $_POST['name'];
                $data = null;
                $fields = " address, perm_address, licence_no, licence_valid_upto";
                $records = mysql_query("select $fields from tblmainmaster where name = '$name' and ac_type = 'Driver' limit 1");
                if (!$records) {
                    die('Error executing query: ' . mysql_error());
                } else {
                    $data = mysql_fetch_assoc($records);
                }
                echo json_encode($data);
            }
        }
    } else {
        echo '{"Error":"Task not defined"}';
        $log->user('TASK not defined' . "\n", $username);
    }
\$\endgroup\$
  • \$\begingroup\$ OOPing this is a bit of a feat, if you'd like I can try to give you some pointers in chat (you need 20 reputation to access Code Review Chat) \$\endgroup\$ – Madara's Ghost Jun 7 '14 at 18:13
4
\$\begingroup\$

HTML

  • Lack of consistent naming conventions and code style. Sometimes you see <script src= "js/drivers.js"> and sometimes id = 'drivers_name'. Pick one, and stick with it. It makes your code that much more readable.
  • Your <form> element lacks the method= and action= attributes.
  • You have redundant code.

    • <label>s must be associated with inputs. So the two labels at the top are not valid.
    • You have too many HTML tags per input, don't forget that you can nest the input inside a label, thus associating it implicitly, and also saving you the need for using IDs for every single input. For example:

      <label class="uk-form-label">Name
          <input type="text" name="driver_name" class="uk-form-small autocomplete_with_new_value_driver PascalCase SelectOnFocus width400">
      </label>
      
    • You don't need <div>s for each label and each input inside of a label. Use display: block; on labels and inputs to your advantage!
    • Don't use <br>s to control vertical spacing. Use margin-top and padding-top to your advantage!
    • You are using redundant attributes, for example <div align="center"> should be a div with text-align: center; set for it in a CSS stylesheet. tabindex is redundant if you code your form properly with order of elements.
  • The reset <button type="reset"> is redundant. No one uses it for any real purpose (I can just refresh) and it's extremely annoying to try and hit the submit button, only to accidentally reset the form. Please don't.

JavaScript

  • You don't need jQuery. At a glance, and especially since you've given IDs for everything, you don't really need jQuery for most of the things you do there.
  • Leverage the use of HTML5's validation. Use Feature detection to determine when to use your validation, and when to let the browser do it for you. It's semantic, and native.
  • Generalize the buttons. Why do you have 3 blocks of code for 3 buttons? They should all submit to the same page, with slightly different parameters. All processing is done at the server, client only displays the results.

PHP

  • Only use include_once or require_once. There are very specific cases when you want to use include or require, connecting (to a database?) is not one of them.
  • Naming convention. $variableNames are always camelCased, ClassNames are always CamelCaps. That's how I do it, adopt one and stick with it.
  • Your MyFunctions class should not be a class. It should be a bunch of functions.
  • That first condition is redundant. You're checking if any of the conditions are true, and then you check them one by one. You can skip the initial check.
  • Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.
  • You are exposed to SQL Injection attacks. This is a critical issue. See How can I prevent SQL injection with PHP.
  • If your PHP file returns a JSON string, don't construct it yourself. You should only have one echo, at the bottom of the file, and that's echo json_encode($results);. Everything you want to echo should be placed into the $results array.
\$\endgroup\$
  • \$\begingroup\$ I'll probably edit this to include the other parts as I read them, Stay tuned! \$\endgroup\$ – Madara's Ghost Jun 7 '14 at 17:26
  • \$\begingroup\$ Thanks a lot for your detailed explanation. I have already started implementation of many of your suggestions. Looking forward for more of your valuable suggestions. Presently I am working on to develop a class which can generate forms and server side php based on the field details passed to it. you advised me to use actions in html titles rather than jquery post. how cam i update the form details w/o reloading the form in that case? please advise me, thanks. \$\endgroup\$ – user3323438 Jun 16 '14 at 10:17
  • 1
    \$\begingroup\$ @user3323438 the two are not mutually exclusive. You can have an action attribute and use AJAX to submit the form. By having a valid action attribute, you can tell jQuery to post to the URL said in the action, and also your form will work without jQuery if needed! \$\endgroup\$ – Madara's Ghost Jun 16 '14 at 10:27

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.