1
\$\begingroup\$

i am currently working on a project that involves me creating a worksheet for storing student test scores and grades.

i wrote a script that retrieves users information and presents it in form of a worksheet. If changes are made to this document it saved the program stores or updates the database.

I need reviews and optimization since i am certain my coding habits are crude and needs some polishing thanks

<?php
//getRecord.php Script
session_start();

$_SESSION['data']['tid'] = $term = $_POST['term'];
$_SESSION['data']['sid'] = $subject = $_POST['subject'];
$_SESSION['data']['sess'] = $session = $_POST['session'];
$_SESSION['data']['class'] = $class = $_POST['class'];

#get assessment records for that class and session
$q = sprintf("SELECT grades.grade_id, concat(fname,' ', lname) 
     as name, test1, test2, test3, exam 
     FROM grades, pupils 
     WHERE pupils.pupil_id = grades.pupil_id 
     AND class_id = %d 
     AND session = '%s'
     AND term_id = %d
     AND subject_id = %d", $class, $session, $term, $subject);
     $r = $dbc->query($q);
?>  
<form method="POST" action="" >
    <table class="worksheet">
    <tr>
        <th width="2%" class="heading"></th>
        <th width="38%">Student Full Name</th>
        <th width="15%">Test 1 score</th>
        <th width="15%">Test 2 score</th>
        <th width="15%">Test 3 score</th>
        <th width="15%">Exam score</th>
    </tr>
<?php
    if ($r->num_rows > 0){  
        $i = 1;
    #if it does exist
    #get the student info for that paarticular subject
    while (list($gradeID, $stdName, $t1, $t2, $t3, $e) =     $r->fetch_array())
    {
    ?>      
    <tr>
    <td class="heading"><?php echo $i++; ?></td>
    <td><?php echo $stdName; ?></td>
    <td><input type="text" name="test1[<?php echo $gradeID; ?>]" value="<?php if(isset($t1)){ echo $t1; }else{ echo 0; } ?>"/></td>
    <td><input type="text" name="test2[<?php echo $gradeID; ?>]" value="<?php if(isset($t2)){ echo $t2; }else{ echo 0; } ?>" /></td>
    <td><input type="text" name="test3[<?php echo $gradeID; ?>]" value="<?php if(isset($t3)){ echo $t3; }else{ echo 0; } ?>" /></td>
    <td><input type="text" name="exam[<?php echo $gradeID; ?>]" value="<?php if(isset($e)){ echo $e; }else{ echo 0; } ?>" /></td>
    </tr>
    <tr>
    <td colspan="6"><input type="hidden" name="action" value="update" /></td></tr>
    <?php
        }   
    }else{

    //get assessment records for that class and session
    $q = sprintf("SELECT students.pupil_id, concat(fname,' ', lname) as name FROM students, pupils WHERE pupils.pupil_id= students.pupil_id 
        AND class_id = %d 
        AND session = '%s'", $class ,$session);

        $r = $dbc->query($q);
        if ($r->num_rows >= 1 ) {
        $i=1;
        //if it does exist
        //get the student info for that paarticular subject
        while (list($stdID, $stdName) = $r->fetch_array()){
        ?>      
        <tr>
        <td class="heading"><?php echo $i++; ?></td>
        <td><?php echo $stdName; ?></td>
        <td><input type="text" name="test1[<?php echo $stdID; ?>]" value="0"/></td>
        <td><input type="text" name="test2[<?php echo $stdID; ?>]" value="0" /></td>
        <td><input type="text" name="test3[<?php echo $stdID; ?>]" value="0" /></td>
        <td><input type="text" name="exam[<?php echo $stdID; ?>]" value="0" /></td>
        </tr>

        <?php
        }
        echo '
            <tr>
           <td colspan="6"><input type="hidden" name="action" value="addnew" /></td>
           </tr>';
        }
    }
    ?>
    </table>
    <input type="submit" value="Submit Record" name="btnSubmit" class="btn-center"/>
    </form>

The setRecord Script

<?php
require ('../includes/connect.php');

if (isset($_POST['btnGet'])){
    require('sub-modules/getRecord.php');
}

if (isset($_POST['btnSubmit'])){

//i am using sample data for now but normally 
//the data are received from session
$term = $_SESSION['data']['tid'];
$subject = $_SESSION['data']['sid'];
$session = $_SESSION['data']['sess'];
$cid = $_SESSION['data']['class'];

$test1 = $_POST['test1'];
$test2 = $_POST['test2'];
$test3 = $_POST['test3'];
$exam = $_POST['exam'];

$query = '';

$insert_data = array();
$success = $failure = 0; 
$total_insertions = 0;

foreach ($test1 as $k => $v)
{
    $Test1 = $test1[$k]; $Test2= $test2[$k]; $Test3=$test3[$k]; $Exam = $exam[$k];

    if (isset($_POST['action']) && $_POST['action'] == 'addnew')
    {
        $query = "INSERT INTO `grades`(`pupil_id`,`subject_id`,`class_id` ,`term_id`, `test1`, `test2`, `test3`, `exam`, `session`) VALUES ($k,$subject,$cid,$term,$Test1,$Test2,$Test3,$Exam, '$session')";
    }else{
        $query = "UPDATE `grades` SET  `test1` = $Test1, `test2` = $Test2, `test3` = $Test3, `exam` = $Exam WHERE grade_id = $k";
    }

//i did this for debugging purpose so ican be able to see if the insertions 
//were complete and also see those ids that were not inserted
    $result= $dbc->query($query) or trigger_error($dbc->error);
    if ($dbc->affected_rows > 0){
        $success += 1;
    }else{
        $failure += 1;
        $insert_data[] = $k;
    }

    $total_insertions += 1;
}

if ($success == $total_insertions){
    echo 'Insertion was complete with no errors';
}elseif ($failure == $total_insertions){
    echo 'Insertion was complete with errors';
}else{
    echo '<p>Some Insertion complete with some errors</p>';
    echo "<p>IDS:";
    foreach ($insert_data as $n){
        echo $n.', ';
    }
        echo '<br>'.$dbc->error;
}
}

if (isset($_POST['action'])){
    $action = $_POST['action'];
    if ($action == 'update'){

    }
    if ($action == 'addnew'){

    }
}

?>

<form method="post" action="">
    <div>
        <label>Class: </label>
        <select name="class" id="class">

    <option value="3">Primary 1</option>
    <option value="4">Primary 2</option>
    <option value="5">Primary 3</option>
    <option value="8">Primary 4</option>
    <option value="9">Primary 5</option>
    <option value="10">Primary 6</option>   
    </select>
 </div>
 <div>
    <label>Subject: </label>
    <select name="subject" id="subject">
    <option value="1">English Language</option>
    <option value="2">Mathematics</option>
    <option value="3">Religious Knowledge</option>
    <option value="4">Civic Education</option>  
</select>
</div>
<div>
    <label>Session: </label>
    <select name="session">
        <option value="2015/2016">2015/2016</option>
     </select>
 </div>
 <div>
     <label>Term:</label>
    <select name="term">
        <option value="1">First Term</option>
        <option value="2">Second Term</option>
        <option value="3">Third Term</option>
    </select>
</div>
<input type="submit" name="btnGet" value="Get Record"/>
</form>

Database

CREATE TABLE IF NOT EXISTS `grades` (
`grade_id` int(11) NOT NULL AUTO_INCREMENT,
`pupil_id` int(11) NOT NULL,
`subject_id` int(11) NOT NULL,
`class_id` int(11) NOT NULL,
`term_id` int(11) NOT NULL,
`test1` int(11) NOT NULL,
`test2` int(11) NOT NULL,
`test3` int(11) NOT NULL,
`exam` int(11) NOT NULL,
`subPosition` tinyint(3) DEFAULT NULL,
`session` char(9) NOT NULL,
`finalize` tinyint(1) NOT NULL DEFAULT '0',
PRIMARY KEY (`grade_id`),
UNIQUE KEY `score_id_UNIQUE` (`grade_id`),
KEY `fk_score_terms1_idx` (`term_id`),
KEY `fk_score_subjects1_idx` (`subject_id`),
KEY `fk_grades_pupils1_idx` (`pupil_id`),
KEY `fk_grades_classes1_idx` (`class_id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 AUTO_INCREMENT=47 ;

--
-- Dumping data for table `grades`
--

INSERT INTO `grades` (
`grade_id`, `pupil_id`, `subject_id`, `class_id`, `term_id`, `test1`, `test2`, `test3`, `exam`, `subPosition`, `session`, `finalize`) VALUES
(27, 7, 1, 3, 1, 10, 10, 0, 70, NULL, '2015/2016', 1),
(28, 3, 1, 3, 1, 0, 0, 0, 0, NULL, '2015/2016', 0),
(29, 8, 1, 3, 1, 0, 0, 0, 0, NULL, '2015/2016', 0),
(30, 9, 1, 3, 1, 0, 0, 0, 0, NULL, '2015/2016', 0),
(31, 10, 1, 3, 1, 0, 5, 0, 0, NULL, '2015/2016', 0),
(32, 7, 2, 3, 1, 5, 7, 10, 0, NULL, '2015/2016', 1),
(33, 3, 2, 3, 1, 6, 9, 10, 0, NULL, '2015/2016', 1),
(34, 8, 2, 3, 1, 5, 7, 5, 0, NULL, '2015/2016', 1),
(35, 9, 2, 3, 1, 7, 7, 7, 0, NULL, '2015/2016', 1),
(36, 10, 2, 3, 1, 9, 9, 8, 0, NULL, '2015/2016', 1),
(42, 11, 2, 4, 1, 5, 10, 0, 0, NULL, '2015/2016', 1),
(46, 11, 1, 4, 1, 4, 10, 7, 0, NULL, '2015/2016', 0);
\$\endgroup\$
1
\$\begingroup\$

Not a full review, just a couple of things I noticed:

Security

All of your queries are vulnerable to SQL Injection. You should use either mysqli_ or PDO with prepared statements.

Your setRecord script is also vulnerable to reflected XSS (echo $n.', '; where n is user supplied).

Your code is also vulnerable to CSRF and stored XSS.

Structure

Your PHP file contains HTML code, CSS code, and PHP code. This is definitely too much. Personally, I would separate it as much as possible.

The first thing I would do is put the CSS info in its own file.

Then, I would introduce functions; for example getStudentsByClass, getGradesByClass, insertGrade, etc. This makes your code reusable, more readable, and means that you can get rid of most of your comments.

You should also introduce functions for duplicate code. For example, instead of this: "<?php if(isset($t1)){ echo $t1; }else{ echo 0; } ?> you might have

function default($value, $default) 
{
    if (isset($value) {
        return $value;
    } else {
        return $default;
    }
}

Then it's just echo default($t1, 0);.

The same way, you might want to put the building of the input tag into a function.

Naming

  • names should all start with a lower-case character. You should never have variable names that only differ in case (eg Test1 vs test1), as it is highly confusing.
  • names should almost never be only one character. r might be gradesResult, e might be exam, and so on.

Style

  • your indention and spacing are partly off. You can use an IDE to fix this.
  • your placement of { is not consistent. It should go on the next line for functions and classes, and on the same line for everything else.

Misc

  • Why do you store some values in a session? This doesn't seem to be needed.
  • Same things should have the same name. English isn't my first language, but is there really a difference between pupils and students?
  • all SQL keywords should be uppercase. You generally follow this rule, but didn't for as.
  • I would get rid of variables that you only use in one or two places. $test1 for example is only used once, and $Test1 only twice. Their name also doesn't add any new information.
  • I would declare variables as late as possible; that way a reader doesn't have to keep them in mind until they are actually necessary.
\$\endgroup\$
  • 1
    \$\begingroup\$ (default is a keyword, probably a bad idea to use that as a function name.) \$\endgroup\$ – Mat Oct 15 '15 at 15:36
  • \$\begingroup\$ @Mat Thanks, that's of course correct. getDefault or defaultValue or something might work instead. \$\endgroup\$ – tim Oct 15 '15 at 16:12
  • \$\begingroup\$ i normally use mysqli prepared statements but i quit on it because most times i cant debug errors with it, especially errors as a result of database queries. i cant see the generated query so that i can see where i am wrong. In security i found out you kept mentioning XSS and CSRF is there any way or any materials that can help me avoid such security loops \$\endgroup\$ – Sylvester hillary Oct 16 '15 at 11:24
  • \$\begingroup\$ @SylvesterHilary With mysqli/PDO, you should check the return values of execute, prepare, etc; that way, you at least find out which query fails (being open to SQL injection is def. not an alternative). You can also enable your mysql error log for debugging, or add a debug variable and echo queries when it is enabled. XSS/CSRF: owasp is always a great resource for general info: XSS, CSRF. for PHP specifics, I would just google it. \$\endgroup\$ – tim Oct 16 '15 at 13:36
  • \$\begingroup\$ @tim how can you echo queries in prepared statement i am trying to do this for an update to debug it but its just a nightmare \$\endgroup\$ – Sylvester hillary Oct 19 '15 at 21:22

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.