7
\$\begingroup\$

I'm programming a system and I want to know if this is the correct way or if there is a better way to do it.

Container:

I have more fields but I only added one to show you the container.

<form method="post" class="form-horizontal" onsubmit="return submitForm();" role="form" id="div-studentStatus">                     
    <div class="form-group">
        <label class="col-sm-3 control-label">Status:</label>
        <div class="col-sm-5 text-primary" id="div-studentStatus">
            <input type="hidden" value="<?php echo  $_POST['id']; ?>" id="studentId">
            <select class='form-control input-sm'  id="studentStatus">
                <option value="1">Activo</option>
                <option value="0">Inactivo</option>
            </select>
        </div>
        <div class="col-sm-3" align="center"><button type="button" class="btn btn-success btn-block btn-sm" id="statusSaveButton">Guardar</button></div>                                    
        <div class="col-sm-1">&nbsp;</div>
    </div>                                                      
</form> 

When the user clicks on statusSaveButton, I append a confirmation button (statusSaveButtonYes and statusSaveButtonNo). I only add the action if the user clicks on YES.

$(document).on('click','#statusSaveButton',function() {
    $('#studentStatus').attr('disabled', 'disabled');
    $('#statusSaveButton').prop('disabled', true);
    var studentStatus = $("#studentStatus").val();
    if ($("#studentStatusConfirm").length) {
        $("#studentStatusConfirm").html('<div class="col-sm-12">Are you sure you want to modify the record?&nbsp;&nbsp;<button type="button" class="btn btn-success btn-xs" id="statusSaveButtonYes" style="width:30px">Yes</button>&nbsp;&nbsp;<button type="button" class="btn btn-danger btn-xs" id="statusSaveButtonNo" style="width:30px">No</button></div>');
        $("#studentStatusConfirm").show();
    } else {
        $("#div-studentStatus").append('<div class="form-group" id="studentStatusConfirm"><div class="col-sm-12">¿Estas seguro que deseas modificar el status del alumno?&nbsp;&nbsp;<button type="button" class="btn btn-success btn-xs" id="statusSaveButtonYes" style="width:30px">Si</button>&nbsp;&nbsp;<button type="button" class="btn btn-danger btn-xs" id="statusSaveButtonNo" style="width:30px">No</button></div></div>');      
    }
}); 
$(document).on('click','#statusSaveButtonYes',function() {
    var studentId = $("#studentId").val();
    var studentStatus = $("#studentStatus").val();  
    $.post("content/studentsAction.php", {action:"editStudentStatus", studentId:studentId, studentStatus:studentStatus}, function(data){
        if (data != "Successful") {
            $("#studentStatusConfirm").html('<div class="col-sm-12 text-danger">'+data+'</div>');
            $("#statusSaveButtons").show();
            $('#studentStatus').removeAttr('disabled');
            $('#statusSaveButton').prop('disabled', false);             
        } else {
            $("#studentStatusConfirm").html('<div class="col-sm-12 text-success">Success.</div>');
            $("#statusSaveButtons").show();
            $('#studentStatus').removeAttr('disabled');
            $('#statusSaveButton').prop('disabled', false);
        }
    });
}); 

Action (content/studentsAction.php):

I have 8 different actions in this page (I only add one): change status, change level, change name, etc. I control the action with the $_POST["action"] variable.

<?php
$regex_studentId = '/^[0-9]{1,12}$/';
$regex_studentStatus = '/^[01]{1}$/';
if (isset($_POST["action"])) {
    if ($_POST["action"] == "editStudentStatus") {
        if ((isset($_POST["studentId"])) and (isset($_POST["studentStatus"]))) {
            if ((preg_match($regex_studentId, $_POST["studentId"])) and (preg_match($regex_studentStatus, $_POST["studentStatus"]))) {
                $updateStudentStatus = updateStudentStatus($_POST["studentId"],$_POST["studentStatus"]);
                if ($updateStudentStatus) {
                    echo "Successful";
                } else {
                    echo "Error.";
                }
            } else {
                echo "Error.";
            }
        } else {
            echo "Error.";
        }
} else {
    echo "Error.";
}
?>

Database:

function updateStudentStatus($studentId,$studentStatus) {
    include ("./businesslogic/dbconnection/cfg.php");
    $db = new PDO('mysql:host='.$server.';dbname='.$db,$db_user,$db_password);
    $string = "update students set student_status = :studentStatus where student_id = :studentId";
    $sql = $db->prepare($string);
    $sql->bindParam(':studentId',$studentId);
    $sql->bindParam(':studentStatus',$studentStatus);   
    if ($sql->execute()) {
        $db = null;
        return TRUE;
    } else {
        $db = null;
        return FALSE;
    }
}
\$\endgroup\$
  • \$\begingroup\$ This: echo $_POST['id']; is vulnerable to XSS. Use htmlspecialchars to prevent XSS. \$\endgroup\$ – tim Jul 3 '15 at 16:39
2
\$\begingroup\$

Variable definition

You sometimes define variables in camelCase and sometimes with underscores. I recommend to use one spelling type only.

PHP

If function updateStudentStatus is not part of a class consider developing a database class that handles it. Another option is to add a save function to each of your Model classes. This makes your code easier to read. For more details check mvc pattern.

In the updateStudentStatus function you establish a new database connection with each call. You can avoid it by promoting the function to a proper class and define a global attribute holding your database connection.

JS

In your javascript you have html code. I recommend to either load it with rest of the dom and hide it by default or load it using ajax. I personally compare it with inline css.

HTML

In html form you have an attribute onsubmit. It should be in a javascript file. I compare it with inline css as well.

"QUOTES"

Also see CodeX answer.

As tim mentioned $_POST and other variables not provided by yourself have to be validated and parsed by htmlspecialchars e.g. to avoid XSS.

A bit about security

I've added this section due to mentioned XSS.

Security is a very complex topic and must not be underestimated. There are a lot of options to implement and grant security. What kind of options you choose depends on your time/budget but also kind of project and data that are going to be stored in your database. I kindly ask you to think wise about the data that will be saved in the database and make researches about proper security measures.

There are a lot of PHP Frameworks. I recommend to take a look into some of them as those provide security standards.

\$\endgroup\$
1
\$\begingroup\$

This portion of code is very messy and has a lot of unnecessary else { echo error }

<?php
$regex_studentId = '/^[0-9]{1,12}$/';
$regex_studentStatus = '/^[01]{1}$/';
if (isset($_POST["action"])) {
    if ($_POST["action"] == "editStudentStatus") {
        if ((isset($_POST["studentId"])) and (isset($_POST["studentStatus"]))) {
            if ((preg_match($regex_studentId, $_POST["studentId"])) and (preg_match($regex_studentStatus, $_POST["studentStatus"]))) {
                $updateStudentStatus = updateStudentStatus($_POST["studentId"],$_POST["studentStatus"]);
                if ($updateStudentStatus) {
                    echo "Successful";
                } else {
                    echo "Error.";
                }
            } else {
                echo "Error.";
            }
        } else {
            echo "Error.";
        }
} else {
    echo "Error.";
}
?>

This is much cleaner and should work just the same:

<?php
$regex_studentId = '/^[0-9]{1,12}$/';
$regex_studentStatus = '/^[01]{1}$/';


if (isset($_POST["action"], $_POST["studentId"], $_POST["studentStatus"]) && $_POST["action"] == "editStudentStatus") {
    if ((preg_match($regex_studentId, $_POST["studentId"])) and (preg_match($regex_studentStatus, $_POST["studentStatus"]))) {
                $updateStudentStatus = updateStudentStatus($_POST["studentId"],$_POST["studentStatus"]);
                if ($updateStudentStatus) {
                    echo "Successful";
                } 
                else {
                    echo "Error";   
                }
            } 

}

I think this will help your work flow and make the code easier to read, not just for you but for anyone else that might need to work with it.

\$\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.