current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have the following code that I know can be written in a better way. Basically I query tables and display the results for each student for the homework that has been marked.

<?php
include 'inc/db_con.php';

mysql_select_db("homework", $con);

$resultcourse = mysql_query("SELECT course_code FROM courses WHERE course_active='1' LIMIT 1");
$rowcode = mysql_fetch_assoc($resultcourse);

$result = mysql_query("SELECT homework, total_mark, student, MAX(student_mark), COUNT(total_mark), student_name FROM homeworkone INNER JOIN students on homeworkone.student = students.student_uid  WHERE homeworkone.homework='RegnTheory' && students.student_course='$rowcode[course_code]' GROUP BY student_name ORDER BY student_name ASC");


function countScore($counting)
{
    if($counting == '2'){
        $counted = '&bull;&bull;';
    }
    else if($counting == '3'){
        $counted = '&bull;&bull;&bull;';
    }
    else
       $counted = '&bull;';
    return $counted;
}
?>
<!DOCTYPE html>
 <html lang="en-GB">
  <head>
   <meta charset=utf-8 />
   <title>ACS Homework Results</title>
   <link rel="stylesheet" href="css/reset_css3.css" />
   <link rel="stylesheet" href="css/results.css" />

  </head>
  <body>
   <header>
    <div class="contentwrap">
     <div class="left">
      <h1>ACS Level 3</h1>
      <p>Homework Results <?php echo        $rowcode['course_code']; ?></p>
     </div>
     <div class="right"></div>
    </div>
   </header>
   <div id="wrapper">
    <div id="clearheader"><!-- used to make room for the #header --></div>

    <div class="contentwrap">
     <section>
      <table>
       <tr>
        <th>Name</th>
        <th>1</th>
        <th>2</th>
        <th>3</th>
        <th>4</th>
        <th>5</th>
       </tr>
<?php
 while($row = mysql_fetch_array($result)){ 
  $score = 0;
  $score2 = 0;
  $score3 = 0;
  $score4 = 0;
  $score5 = 0;
  $score6 = 0;
  $score7 = 0;
  $score8 = 0;
  $score8b = 0;
  $score9 = 0;
  $score10 = 0;
  $score11 = 0;
  $score12 = 0;
  $score13 = 0;
  $score14 = 0;
  $score15 = 0;
  $score16 = 0;
  $score17 = 0;
  $score18 = 0;
  $score19 = 0;

 if($row['total_mark']!=0){
  $scorefirst = (100 / $row['total_mark']) * $row['MAX(student_mark)'];
  $score = number_format($scorefirst, 1, '.', '');
  $counter = $row['COUNT(total_mark)'];
  $scoreCounter = countScore($counter);
 }
$stu = $row['student'];

$result2 = mysql_query("SELECT homework, student, total_mark, MAX(student_mark),   COUNT(total_mark) FROM homeworkone WHERE homework='OPtoBCP' && student='$stu'");
while($row2 = mysql_fetch_assoc($result2))
{
 if($row2['total_mark']!=0){     
  $scorefirst2 = (100 / $row2['total_mark']) * $row2['MAX(student_mark)'];
  $score2 = number_format($scorefirst2, 1, '.', ''); }
 $counter = $row2['COUNT(total_mark)'];
 $scoreCounter2 = countScore($counter);
}

$result3 = mysql_query("SELECT homework, student, total_mark, MAX(student_mark), COUNT(total_mark) FROM homeworkone WHERE homework='BCPtoGUN' && student='$stu' GROUP BY student");
while($row3 = mysql_fetch_assoc($result3))
{
 if($row3['total_mark']!=0){
  $scorefirst3 = (100 / $row3['total_mark']) * $row3['MAX(student_mark)'];
  $score3 = number_format($scorefirst3, 1, '.', ''); }}

$result4 = mysql_query("SELECT homework, student, total_mark, MAX(student_mark), COUNT(total_mark) FROM homeworkone WHERE homework='FOOptions' && student='$stu' GROUP BY student");
while($row4 = mysql_fetch_assoc($result4))
{
 if($row4['total_mark']!=0){
  $scorefirst4 = (100 / $row4['total_mark']) * $row4['MAX(student_mark)'];
  $score4 = number_format($scorefirst4, 1, '.', ''); }}

$result5 = mysql_query("SELECT homework, student, total_mark, MAX(student_mark), COUNT(total_mark) FROM homeworkone WHERE homework='CheckMap' && student='$stu' GROUP BY student");
while($row5 = mysql_fetch_assoc($result5))
{
 if($row5['total_mark']!=0){
  $scorefirst5 = (100 / $row5['total_mark']) * $row5['MAX(student_mark)'];
  $score5 = number_format($scorefirst5, 1, '.', ''); }}
?>
       <tr height="45px">
        <td><h3><?php echo $row['student_name']; ?></h3></td>
        <td><?php echo $score; ?>&#37;<br /><?php echo $scoreCounter ?></td>
        <td><?php echo $score2; ?>&#37;<br /><?php echo $scoreCounter2 ?></td>
        <td><?php echo $score3; ?>&#37;</td>
        <td><?php echo $score4; ?>&#37;</td>
        <td><?php echo $score5; ?>&#37;</td>
       </tr>
<?php   }
?>

      </table>
     </section>
    </div>
   </div>
   <div id="results"></div>
  </body>
</html>
share|improve this question
    
I am thinking that you might want to look at an array for the $score## variables, not sure what all you are doing with those, but it looks like a good spot for an array. it would clean up a little bit of code there, maybe –  Malachi Oct 7 '13 at 19:33
add comment

1 Answer

up vote 1 down vote accepted

Your queries are only different in the type of homework they look for. Also the processing of the data seems common. The basic way to refactor code like this is to extract it into a method:

function getHomeworkScore($homework, $student)
{
    $score = 0;
    $result = mysql_query("SELECT homework, student, total_mark, MAX(student_mark) as max_mark, COUNT(total_mark) as total_mark_count FROM homeworkone WHERE homework='$homework' && student='$student' GROUP BY student");

     while ($row = mysql_fetch_assoc($result))
     {
         if ($row['total_mark'] != 0)
         {
             $scorefirst = (100 / $row['total_mark']) * $row['max_mark'];
             $score = number_format($scorefirst, 1, '.', ''); 
          }
      }

      return $score;
}

Then you can call it:

$score = getHomeworkScore("OPtoBCP", $stu);
$score2 = getHomeworkScore("BCPtoGUN", $stu);
...

Some notes:

  1. You really want to look into PDO or mysqli otherwise you leave yourself open for a world of pain (SQL injection attacks).

  2. Give your summary column names so you can easily reference them

share|improve this answer
    
Thank you will try this and will look into PDO –  WayneB Oct 8 '13 at 9:06
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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