3
\$\begingroup\$

I'm developing a system in which users can upload their .doc file and the system will check whether the file contains certain words, and the system will tell the users whether their doc file falls under 'Sustainability' category.

The system will find any word in doc file and match it with the keyword that have been predefined in the database and generate the result in percentage form.

The problem I'm having at the moment is that when the doc file is uploaded and when I click the 'Generate' button for the system to process it, it takes forever to generate the result, even though the doc file that I upload is only 12kb.

Based on my observation, the problem starts at ScreenDocument(), because the system can successfully upload the .doc file to the htdocs folder.

 public function ValidateDocument($FileName, $UploadName)
   {
    $PrevUploadName = basename($this->RequestFields->GetField('UploadName'));
    if(strlen($PrevUploadName) > 0 && file_exists(dirname(__FILE__) . '/uploads/' . $PrevUploadName) && is_file(dirname(__FILE__) . '/uploads/' . $PrevUploadName)) 
    unlink(dirname(__FILE__) . '/uploads/' . $PrevUploadName);
    $this->RequestFields->SetField('FileName', basename($FileName));
    $this->RequestFields->SetField('UploadName', basename($UploadName));

    return true;
   }

 public function ScreenDocument()
 {
  //validate input data
  $UploadName = basename($this->RequestFields->GetField('UploadName'));
  $UploadFileName = dirname(__FILE__) . '/uploads/' . $UploadName;
  if(!(strlen($UploadFileName) > 0 && is_file($UploadFileName) &&         
  file_exists($UploadFileName) && filesize($UploadFileName) > 0))     
{
    echo json_encode(array('Error' => true, 'Message' => 'Invalid   document.'));
    return; 
}
$University = (int)@$_POST['University'];
if($University <= 0)
{
    //echo json_encode(array('Error' => true, 'Message' => 'Please select a university!'));
    //return;   
    $University = 1;
}
$Course = (int)@$_POST['Course'];
if($Course <= 0)
{
    //echo json_encode(array('Error' => true, 'Message' => 'Please select a course!'));
    //return;
        $Course = 0;
}
$Subject = @$_POST['Subject'];
if(strlen($Subject) == 0)
{
    echo json_encode(array('Error' => true, 'Message' => 'Please enter a subject!'));
    return; 
}
$FileName = basename($this->RequestFields->GetField('FileName'));
if(strlen($FileName) == 0)
{
    echo json_encode(array('Error' => true, 'Message' => 'Can\'t find the upload file name!'));
    return; 
}
if(($File = fopen($UploadFileName, 'r')) === false)
    return false;
$RawText = @f($File, filesize($UploadFileName)); 
fclose($File);
// document text
$DocHelper = new DocHelper();
$Text = $DocHelper->($UploadFileName);
unset($DocHelper);
//clean the text
$Text = preg_replace('/(\r\n|\r|\n)/', ' ', $Text);
$Text = preg_replace('/[^[:alnum:]\.;\?\+\-]/', ' ', $Text);
$Text = preg_replace('/\+{2,}/', '+', $Text);
$Text = preg_replace('/\-{2,}/', '-', $Text);
$Text = preg_replace('/\?{2,}/', '?', $Text);
$Text = preg_replace('/\;{2,}/', ';', $Text);
$Text = preg_replace('/\.{2,}/', '.', $Text);
$Text = preg_replace('/\s{2,}/', ' ', $Text);
//start a transaction
if(!$this->Db->Handler->query("START TRANSACTION"))
{
    echo json_encode(array('Error' => true, 'Message' => 'Failed to start a transaction!'));
    return;
}
//insert the document in database
$Stmt = $this->Db->Handler->prepare('insert into `documents` (`idmember`, `university`, `course`, `subject`, `filename`, `text`, `data`) values (?, ?, ?, ?, ?, ?, ?)');
if(!$Stmt)
{
    $this->Db->Handler->query("ROLLBACK");
    echo json_encode(array('Error' => true, 'Message' => 'Failed to prepare a statement!'));
    return;
}
$Stmt->bind_param('iiissss', $_SESSION['idmember'], $University, $Course, $Subject, $FileName, $Text, $RawText);
if(!$Stmt->execute())
{
    $this->Db->Handler->query("ROLLBACK");
    echo json_encode(array('Error' => true, 'Message' => 'Failed to insert the document into database!'));
    return;
}
$DocumentId = $Stmt->insert_id;
$Stmt->close();
unset($Stmt);
//screen for the first 8 criteria
//LogTrace($Text);
$CriteriaHits = array();
$IsSustainable = false;
$TotalNumberOfKeywords = 0;
for($i = 1; $i <= 8; $i++)
{
    $CriteriaHits[$i] = array();
    $CriteriaHits[$i]['TotalKeywordCount'] = 0;
    $CriteriaHits[$i]['CurrentDistinctHitCount'] = 0;
    $CriteriaHits[$i]['Hits'] = array();
    $Result = $this->Db->OpenQuery('select `id`, `criteria`, `value` from `keywords` where `criteria` = ' . $i . ' order by `value` asc');
    if($Result && $this->Db->GetQueryRecordCount($Result) > 0)
    {
        $TotalKeywordCount = 0;
        while($Row = $this->Db->GetQueryNextRow($Result))
        {
            $KeywordMatched = false;
            $Offset = 0;
            while(preg_match('/\b' . $Row['value'] . '\b/i' , $Text, $Matches, PREG_OFFSET_CAPTURE, $Offset) == 1)
            {
                $Hit = array();
                $Hit['id'] = $Row['id'];
                $Hit['criteria'] = $Row['criteria'];
                $Hit['value'] = $Row['value'];
                $Hit['hittext'] = $Matches[0][0];
                $Hit['hitoffset'] = $Matches[0][1];
                $CriteriaHits[$i]['Hits'][] = $Hit;
                $Offset = $Matches[0][1] + 1;
                $IsSustainable = true;
                $KeywordMatched = true;
            }
            $TotalKeywordCount++;
            if($KeywordMatched)
                $CriteriaHits[$i]['CurrentDistinctHitCount']++; 
            unset($Row);
        }
        $CriteriaHits[$i]['TotalKeywordCount'] = $TotalKeywordCount;
        $TotalNumberOfKeywords += $TotalKeywordCount;
    }
    $this->Db->CloseQuery($Result);
}
//is sustainable ?
if(!$IsSustainable)
{
    $this->Db->Handler->query("COMMIT");
    echo json_encode(array('Error' => false, 'Message' => 'Document is not sustainable.'));
    return;
}
//get the criteria names
$CriteriaNames = array();
$Result = $this->Db->OpenQuery('select `id`, `name` from `criteria` order by `id` asc');
if($Result && $this->Db->GetQueryRecordCount($Result) > 0)
{
    while($Row = $this->Db->GetQueryNextRow($Result))
    {
        $CriteriaNames[$Row['id']] = $Row['name'];
    }
}
$this->Db->CloseQuery($Result);
//complete the screening for the rest of criteria
for($i = 9; $i <= count($CriteriaNames); $i++)
{
    $CriteriaHits[$i] = array();
    $CriteriaHits[$i]['TotalKeywordCount'] = 0;
    $CriteriaHits[$i]['CurrentDistinctHitCount'] = 0;
    $CriteriaHits[$i]['Hits'] = array();
    $Result = $this->Db->OpenQuery('select `id`, `criteria`, `value` from `keywords` where `criteria` = ' . $i . ' order by `value` asc');
    if($Result && $this->Db->GetQueryRecordCount($Result) > 0)
    {
        $TotalKeywordCount = 0;
        while($Row = $this->Db->GetQueryNextRow($Result))
        {
            $KeywordMatched = false;
            $Offset = 0;
            while(preg_match('/\b' . $Row['value'] . '\b/i' , $Text, $Matches, PREG_OFFSET_CAPTURE, $Offset) == 1)
            {
                $Hit = array();
                $Hit['id'] = $Row['id'];
                $Hit['criteria'] = $Row['criteria'];
                $Hit['value'] = $Row['value'];
                $Hit['hittext'] = $Matches[0][0];
                $Hit['hitoffset'] = $Matches[0][1];
                $CriteriaHits[$i]['Hits'][] = $Hit;
                unset($Hit);
                $Offset = $Matches[0][1] + 1;
                $KeywordMatched = true;
            }
            $TotalKeywordCount++;
            if($KeywordMatched)
                $CriteriaHits[$i]['CurrentDistinctHitCount']++; 
            unset($Row);
        }
        $CriteriaHits[$i]['TotalKeywordCount'] = $TotalKeywordCount;
        $TotalNumberOfKeywords += $TotalKeywordCount;
    }
    $this->Db->CloseQuery($Result);
}
//LogTrace($CriteriaHits);
//var_dump($CriteriaHits);
//prepare the insert stmt for keywords
$Stmt = $this->Db->Handler->prepare('insert into `results` (`idmember`, `document`, `criteria`, `offset`, `keyword`) values (?, ?, ?, ?, ?)');
if(!$Stmt)
{
    $this->Db->Handler->query("ROLLBACK");
    echo json_encode(array('Error' => true, 'Message' => 'Failed to prepare a statement!'));
    return;
}
$Stmt->bind_param('iiiii', $_SESSION['idmember'], $DocumentId, $Criteria, $Offset, $Keyword);
foreach($CriteriaHits as $Idx => $Criteria)
{
    foreach($Criteria['Hits'] as $Hit)
    {
        $Criteria = $Hit['criteria'];
        $Offset = $Hit['hitoffset'];
        $Keyword = $Hit['id'];
        //insert each keyword found into database
        if(!$Stmt->execute())
        {
            $this->Db->Handler->query("ROLLBACK");
            echo json_encode(array('Error' => true, 'Message' => 'Failed to insert keywords into database!'));
            return;
        }
    }
}
$Stmt->close();
//remove any non-alpha characters from text
$Text = preg_replace('/[^[:alnum:]]/', ' ', $Text);
$Text = preg_replace('/\s{2,}/', ' ', $Text);
//now create a array of words from this text
$Text = explode(' ', trim($Text));
//
$Stmt = $this->Db->Handler->prepare('insert into `criteria_match` (`idmember`, `document`, `criteria`, `percentage`) values (?, ?, ?, ?)');
if(!$Stmt)
{
    $this->Db->Handler->query("ROLLBACK");
    echo json_encode(array('Error' => true, 'Message' => 'Failed to prepare a statement!'));
    return;
}
$Stmt->bind_param('iiid',$_SESSION['idmember'], $DocumentId, $CriteriaIdx, $Percent);
//create the output     
$TotalCount = 0;
$TotalDistinctCount = 0;
$TotalCriteriaMatched = 0;
$TableHtml = '';
foreach($CriteriaHits as $Idx => $Criteria)
{
    $CriteriaIdx = $Idx;
    $Percent = RoundUp($Criteria['CurrentDistinctHitCount'] / $Criteria['TotalKeywordCount'] * 100, 2);
    //echo "alaa:".$Criteria['CurrentDistinctHitCount']."</br>";
    //echo"iihh: " . $Criteria['TotalKeywordCount'];
//$Percent = RoundUp(count($Criteria['Hits']) / count($Text) * 100, 2);
    //insert each keyword found into database
if(!$Stmt->execute())
    {
        $this->Db->Handler->query("ROLLBACK");
        echo json_encode(array('Error' => true, 'Message' => 'Failed to insert keywords into database!'));
        return;
    }
    //
    $TableHtml .= '<tr>';
    if($Idx == 1)
        $TableHtml .= '<td rowspan="8" style="vertical-align: middle;">SD-ESD</td>';
    else if($Idx == 9)
        $TableHtml .= '<td rowspan="10" style="vertical-align: middle;">WEHAB</td>';
    else if($Idx == 19)
        $TableHtml .= '<td rowspan="6" style="vertical-align: middle;">+3</td>';
    $TableHtml .= '<td>' . $CriteriaNames[$Idx] . '</td>';
    $TableHtml .= '<td>' . $Percent . '%</td>';
    $TableHtml .= "</tr>\n";
    //$TableHtml .= '<tr><td></td><td>';
    $PrevWord = '';
    $CurrentCount = 0;
    foreach($Criteria['Hits'] as $Hit)
    {
        if($PrevWord == '')
            $PrevWord = $Hit['value'];
        else if($PrevWord != $Hit['value'] && $CurrentCount > 0)
        {
            //$TableHtml .= '<p>&quot;' . $PrevWord . '&quot; - ' . $CurrentCount . ' match' . ($CurrentCount != 1 ? 'es' : '') . "</p>\n";
            $PrevWord = $Hit['value'];
            $CurrentCount = 0;
        }
        $CurrentCount++;
    }
    if($CurrentCount > 0)
    {
        //$TableHtml .= '<p>&quot;' . $PrevWord . '&quot; - ' . $CurrentCount . ' match' . ($CurrentCount != 1 ? 'es' : '') . "</p>\n";
        $TotalDistinctCount++;
    }
    $TotalCount += count($Criteria['Hits']);
    $TotalDistinctCount += $Criteria['CurrentDistinctHitCount'];
    if(count($Criteria['Hits']) > 0)
        $TotalCriteriaMatched++;
    //$TableHtml .= "</td><td></td></tr>\n";
}
$Stmt->close();
//commit this transaction
$this->Db->Handler->query("COMMIT");
//$tot = ($Percent /24 )*100;
$Html  = "<div class=\"SamResultTabsDiv\">
            <ul>
                <li><a href=\"historytable.php?id=$DocumentId\" target=\"frame\"><img src='images/table2.png' height='25' width='25' /> TABLE</a></li>
                <li><a href=\"histogramGenerate.php?id=$DocumentId&chart=column\" target=\"frame\" ><img src='images/histogram.png' height='25' width='25' /> Column Chart</a></li>
                <li><a href=\"histogramGenerate.php?id=$DocumentId&chart=pie\" target=\"frame\" ><img src='images/piechart.png' height='25' width='25' /> Pie Chart</a></li>
                <li><a href=\"histogramGenerate.php?id=$DocumentId&chart=scatter\" target=\"frame\" ><img src='images/scatterchart.png' height='25' width='25' /> Scatter Chart</a></li>
                <li><a href=\"histogramGenerate.php?id=$DocumentId&chart=spider\" target=\"frame\" ><img src='images/spiderchart.png' height='25' width='25' /> Spider Chart</a></li>
            </ul>
        </div>";
$Html .= "<p align=\"left\"><iframe src=\"blank.php\" name=\"frame\" height=\"900px\" width=\"700px\" frameborder=\"0\" marginheight=\"0\" marginwidth=\"0\" scrolling=\"no\"><p>Your browser does not support iframes.</p></iframe></p>";
/*$Html .= "<div class=\"SamTableDiv\" id=\"SamTableDiv\">\n";
$Html .= "  <div class=\"TableLegend\">Results</div>\n";
$Html .= "  <div class=\"ResultStats\">\n";
$Html .= "      <p>Percentage total of matched keyword = " ."$tot". "%</p>\n";
$Html .= "      <p>Percentage based on keywords = " . RoundUp($TotalDistinctCount / ($TotalNumberOfKeywords / 100), 2) . "%</p>\n";
$Html .= "      <p>Percentage based on criteria = " . RoundUp($TotalCriteriaMatched / (count($CriteriaNames) / 100), 2) . "%</p>\n";
$Html .= "  </div>\n";
$Html .= "  <table>\n";
$Html .= "      <thead><th>TYPE</th><th>CRITERIA</th><th>SUSTAINABILITY RESULT(%)</th></tr>\n";
$Html .= "      <tbody>" . $TableHtml . "</tbody>\n";
$Html .= "  </table>\n";
$Html .= "</div>\n";
$Html .= "<div class=\"SamHistogramDiv\" id=\"SamHistogramDiv\">\n";
$Html .= "<iframe src=\"histogram.php?id=$DocumentId\" name=\"frame\" height=\"700px\" width=\"700px\" frameborder=\"0\" marginheight=\"0\" marginwidth=\"0\" scrolling=\"auto\"><p>Your browser does not support iframes.</p></iframe>";
$Html .= "</div>\n";
*/
?>
<?php
include 'configure.php';
$idmembersession = $_SESSION['idmember'];
//-------------------------CALCULATION: 3 kategory resul---------------------------//
$sd=8;
$query2 = "select SUM(percentage) as total from criteria_match where idmember=$idmembersession and document=$DocumentId and criteria<=$sd" ;
$sql_query2 = mysql_query($query2) or die('Error 3 :'.mysql_error());
$totalcrcountsd=0;
while($data2 = mysql_fetch_assoc($sql_query2,MYSQL_ASSOC))
{
    $totalcrcountsd= $data2['total'];
}
$we=18;
$totalcrcountwe=0;
$query3 = "select SUM(percentage) as total1 from criteria_match where idmember=$idmembersession and document=$DocumentId and criteria<=$we" ;
$sql_query3 = mysql_query($query3) or die('Error 3 :'.mysql_error());
$wecount=0;
while($data3 = mysql_fetch_assoc($sql_query3,MYSQL_ASSOC))
{
    $wecount = $data3['total1'];
    $totalcrcountwe=($wecount-$totalcrcountsd);
}
$tre=24;
$totalcrcounttre=0;
$query4 = "select SUM(percentage) as total2 from criteria_match where idmember=$idmembersession and document=$DocumentId and criteria<=$tre" ;
$sql_query4 = mysql_query($query4) or die('Error 3 :'.mysql_error());
$trecount=0;
while($data4 = mysql_fetch_assoc($sql_query4,MYSQL_ASSOC))
{
    $trecount = $data4['total2'];
    $totalcrcounttre=($trecount-$totalcrcountwe-$totalcrcountsd);
}
$totalcrcountsd=$totalcrcountsd/8;
$totalcrcountwe=$totalcrcountwe/10;
$totalcrcounttre=$totalcrcounttre/6;
//-------------------------CALCULATION: Blink2 total everage result---------------------------//
$query1  = "select SUM(percentage) as total from criteria_match where idmember=$idmembersession and document=$DocumentId";
$sql_query1 = mysql_query($query1) or die('Error 3 :'.mysql_error());
$TotalCount = 0;
while($data1 = mysql_fetch_assoc($sql_query1,MYSQL_ASSOC))
    {
        $TotalCount = $data1['total'];
    }
    mysql_free_result($sql_query1);
$tot= ($TotalCount / 24);
            //-------------------------(CALCULATION: Merge the result from 24 criteria to criteria---------------------------//
$query24  = "select * from criteria_match where idmember=$idmembersession and document=$DocumentId";
$sql_query24 = mysql_query($query24) or die('Error 3 :'.mysql_error());
$criteria1 = 0; $cr2 = 0; $criteria2=0; $cr3=0; $criteria3=0; $cr4 = 0; $criteria4=0; 
$cr5=0; $criteria5=0; $cr6=0; $criteria6=0; $cr7=0; $criteria7=0; $cr8=0; $criteria8=0;
$cr9=0; $criteria9=0; $cr10=0; $criteria10=0; $cr11=0; $criteria11=0; $cr12=0; $criteria12=0;
$Tcriteria1= 0; $Tcriteria2=0; $Tcriteria3=0; $Tcriteria4=0; $Tcriteria5=0; $Tcriteria6=0; $Tcriteria7=0;
$Tcriteria8=0; $Tcriteria9=0; $Tcriteria10=0; $Tcriteria11=0; $Tcriteria12=0;
while($data24 = mysql_fetch_assoc($sql_query24,MYSQL_ASSOC))
    {
        $criteria24 = $data24['criteria'];
        $percentage24 = $data24['percentage'];
        if ($criteria24<=2)
        {
            $criteria1=$criteria1 + $percentage24;  
            $Tcriteria1=Round( $criteria1 / 2, 2 );
        }
        if ($criteria24<=4)
        {
            $cr2=$cr2+$percentage24;
            $criteria2=$cr2-$criteria1; 
            $Tcriteria2=Round($criteria2 / 2, 2 );
        }
        if ($criteria24<=6)
        {
            $cr3=$cr3+$percentage24;
            $criteria3=$cr3-$criteria1-$criteria2;
            $Tcriteria3=Round($criteria3/2,2);  
        }
        if ($criteria24<=8)
        {
            $cr4=$cr4+$percentage24;
            $criteria4=$cr4-$criteria1-$criteria2-$criteria3;
            $Tcriteria4=Round($criteria4/2,2);
        }
        if ($criteria24<=10)
        {   
            $cr5=$cr5+$percentage24;
            $criteria5=$cr5-$criteria1-$criteria2-$criteria3-$criteria4;
            $Tcriteria5=Round($criteria5/2,2);
        }
        if ($criteria24<=12)
        {   
            $cr6=$cr6+$percentage24;
            $criteria6=$cr6-$criteria1-$criteria2-$criteria3-$criteria4-$criteria5;
            $Tcriteria6=Round($criteria6/2,2);
        }
        if ($criteria24<=14)
        {   
            $cr7=$cr7+$percentage24;
            $criteria7=$cr7-$criteria1-$criteria2-$criteria3-$criteria4-$criteria5-$criteria6;
            $Tcriteria7=Round($criteria7/2,2);
        }
        if ($criteria24<=16)
        {
            $cr8=$cr8+$percentage24;
            $criteria8=$cr8-$criteria1-$criteria2-$criteria3-$criteria4-$criteria5-$criteria6-$criteria7;
            $Tcriteria8=Round($criteria8/2,2);
        }
        if ($criteria24<=18)
        {   
            $cr9=$cr9+$percentage24;
            $criteria9=$cr9-$criteria1-$criteria2-$criteria3-$criteria4-$criteria5-$criteria6-$criteria7-$criteria8;
            $Tcriteria9=Round($criteria9/2,2);
        }
        if ($criteria24<=20)
        {   
            $cr10=$cr10+$percentage24;
            $criteria10=$cr10-$criteria1-$criteria2-$criteria3-$criteria4-$criteria5-$criteria6-$criteria7-$criteria8-$criteria9;
            $Tcriteria10=Round($criteria10/2,2);
        }
        if ($criteria24<=22)
        {   
            $cr11=$cr11+$percentage24;
            $criteria11=$cr11-$criteria1-$criteria2-$criteria3-$criteria4-$criteria5-$criteria6-$criteria7-$criteria8-$criteria9-$criteria10;
            $Tcriteria11=Round($criteria11/2,2);
        }
        if ($criteria24<=24)
        {   
            $cr12=$cr12+$percentage24;
            $criteria12=$cr12-$criteria1-$criteria2-$criteria3-$criteria4-$criteria5-$criteria6-$criteria7-$criteria8-$criteria9-$criteria10-$criteria11;
            $Tcriteria12=Round($criteria12/2,2);
        }
    }   
$query = "INSERT INTO `compareresult`(`idmember`,`iddocument`,`subject`,`result`,`resultsd`,`resultwe`,`resulttre`,
`c1`,`c2`,`c3`,`c4`,`c5`,`c6`,`c7`,`c8`,`c9`,`c10`,`c11`,`c12`) 
            VALUES ('$idmembersession','$DocumentId','$Subject','$tot','$totalcrcountsd','$totalcrcountwe','$totalcrcounttre',
            '$Tcriteria1','$Tcriteria2','$Tcriteria3','$Tcriteria4','$Tcriteria5','$Tcriteria6',
            '$Tcriteria7','$Tcriteria8','$Tcriteria9','$Tcriteria10','$Tcriteria11','$Tcriteria12')";
            mysql_query($query) or die("Error 3: ".mysql_error());      
        echo json_encode(array('Error' => false, 'Message' => 'Document data generated.', 'Html' => $Html));}
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Asking for performance improvements is always better done when you show that you have identified one or more bottlenecks. Help us help you, add calls to microtime() at key points, log the results, and edit the post to tell reviewers where to start looking. Other than that, note reviewers are free to comment on any & all aspects of the code, which may or may not improve performance. Good luck! \$\endgroup\$ – Mathieu Guindon Jul 27 '15 at 2:10
  • \$\begingroup\$ HI, I am also new to Code Review, but not new to PHP. That said one obvious thing is the code looks very procedural in nature. There are quite a few loops present. Without really diving into it. It's hard to say, but simplifying or reducing some of this complexity could go a long way to improving the speed. I also see a number of database operations, which can make optimizing very hard without the actual database schema. As posted before some simple bench marking may go a long way to start that process. \$\endgroup\$ – ArtisticPhoenix Jul 27 '15 at 23:32
  • \$\begingroup\$ @ArtisiticPhoenix Feel free to dig in it and make it an answer. Seems you know what the problem is and how to solve it. \$\endgroup\$ – Mast Jul 28 '15 at 11:13
2
\$\begingroup\$

Well for starters here is a good example

 public function ValidateDocument($FileName, $UploadName){
    $PrevUploadName = basename($this->RequestFields->GetField('UploadName'));
    if(strlen($PrevUploadName) > 0 && file_exists(dirname(__FILE__) . '/uploads/' . $PrevUploadName) && is_file(dirname(__FILE__) . '/uploads/' . $PrevUploadName)) 
    unlink(dirname(__FILE__) . '/uploads/' . $PrevUploadName);
    $this->RequestFields->SetField('FileName', basename($FileName));
    $this->RequestFields->SetField('UploadName', basename($UploadName));

    return true;
  }

 public function ScreenDocument(){
  //validate input data
  $UploadName = basename($this->RequestFields->GetField('UploadName'));
  $UploadFileName = dirname(__FILE__) . '/uploads/' . $UploadName;
  if(!(strlen($UploadFileName) > 0 && is_file($UploadFileName) &&         
  file_exists($UploadFileName) && filesize($UploadFileName) > 0)){
    echo json_encode(array('Error' => true, 'Message' => 'Invalid   document.'));
    return; 
}

$University = (int)@$_POST['University'];
if($University <= 0)
{
    //echo json_encode(array('Error' => true, 'Message' => 'Please select a university!'));
    //return;   
    $University = 1;
}

There is just a bunch of duplicated code, and checks upon checks here. To be honest if this isn't part of a class, I don't see how using $this is valid PHP. It's possible this is part of a class, but I can't see that by looking at what is posted. Not to mention the part just hanging out where it should be in a method really says that it's not a class.

So although I would like to dig in it and make it an answer there is just too much going on that looks like programing errors for me to sensibly attempt it. That, or it's just too much of a mess and missing essential parts. To make heads or tails of it.

But just taking that bit. I would combine these it seems the purpose is validation of a file. by using dependance injection. Like this.

 public function ValidateDocument($FileName, $UploadName){
    $PrevUploadName = basename($this->RequestFields->GetField('UploadName'));  ///<---external hidden dependency on RequestFields object and whatever 'UploadName' is probably some input field.

There is an attempt here at dependency injection, but the it's given up on the first line. So what dependency injection is all about is making your method or what have you only know about what you pass into them. In some cases that's not sensible in a class, but if you are going to do general validation then it is. Personally I would do the validation before submitting the data for processing to a class but that's assuming this is a class.

It's much better to limit the scope of these things and just validate the file in one place and set it else where.

I really am sorry it's just to much, take this example

   if(strlen($PrevUploadName) > 0 && file_exists(dirname(__FILE__) . '/uploads/' . $PrevUploadName) && is_file(dirname(__FILE__) . '/uploads/' . $PrevUploadName)) 
    unlink(dirname(__FILE__) . '/uploads/' . $PrevUploadName);
    $this->RequestFields->SetField('FileName', basename($FileName));
    $this->RequestFields->SetField('UploadName', basename($UploadName));

    return true;

Because of the lack of {} this will set FileName and UploadName and return true no matter what the outcome of the if statement is, I really doubt that is the intent, perhaps it is. How can I tell be looking at this when things are being pulled in an checked that have nothing to do with the inputs of this method? / funciton? This is only like the first 10% of this code and there are already several critical problems, surly it's so hard to read that making picking through it a major chore.

Looking at it more, it seems that ScreenDocument just continues on and on to some unknown part of this code, maybe all the way to the end. It needs to be broken down into it's constituent parts.

The smallest example I can do on what to improve is stuff like this

$University = (int)@$_POST['University'];
if($University <= 0){
    //echo json_encode(array('Error' => true, 'Message' => 'Please select a university!'));
    //return;   
    $University = 1;
}

Can be cleaned up like this

$University = isset($_POST['University']) ? intval($_POST['University']) : 0;
if(!$University){   
    $University = 1;
}

I've removed the casting and error suppression both of which are bad for performance.

This bit

//start a transaction
if(!$this->Db->Handler->query("START TRANSACTION"))
{
    echo json_encode(array('Error' => true, 'Message' => 'Failed to start a transaction!'));
    return;
}
//insert the document in database
$Stmt = $this->Db->Handler->prepare('insert into `documents` (`idmember`, `university`, `course`, `subject`, `filename`, `text`, `data`) values (?, ?, ?, ?, ?, ?, ?)');
if(!$Stmt)
{
     .....

Is this PDO, mysqli I don't know. If its pdo we could trim all this down with a simple try catch

 $this->Db->Handler->query("START TRANSACTION");
 try{
       $Stmt = $this->Db->Handler->prepare('insert into `documents` (`idmember`, `university`, `course`, `subject`, `filename`, `text`, `data`) values (?, ?, ?, ?, ?, ?, ?)');
   //... rest of db operation all in one try catch.
 }catch ( \PDOException $e ){
      $this->Db->Handler->query("ROLLBACK");
      echo json_encode(array('Error' => true, 'Message' => $e->getMessage()));
      exit;  //json return - shouldn't we just stop here?
 }

So in summery, I don't know where classes start and end. Where any of this mystical RequestFields are coming from. I'm sure it makes sense given the whole class, but I don't have that. There is to much missing information that is essential, and so much more that need not be here. Where do methods start or stop. What class(s) am I dealing with here. In essence it's not proper OOP, sorry not to be rude or mean but that is just the plain truth IMO. It's built for one purpose and one purpose only, to handle input from a form, and unless this is part of a controller in an MVC framework than it makes little sense to me.

\$\endgroup\$
  • \$\begingroup\$ @ArtisticPhoenix I can show you the whole files related to this php file. A few days back I tried uploading .pdf file and the system generated the results as soon as I clicked on the 'Generate' button. Only .doc file takes some time to be processed. I guess I have to change the requirement to just allow user to upload pdf file only. \$\endgroup\$ – Fizzi Jul 29 '15 at 1:02

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.