Eli
Eli

Reputation: 4359

how can i make my php code shorter?

can some one please give me advice on how I make this block of code considerably shorter?

What it does is get data from a text file uploaded onto the server. It reads the file and it looks for 3 keywords. I placed each keyword in an array, then it outputs the entire line where the keyword is found in. Then in the foreach statemens i loops through it and echo out the line (for debugging purposes, but when complete I would not like it to echo the line but rather just do the count) then it counts all the instances of the keyword. In the end I echo out the number of occurances. I feel that this can be done more efficiently and organized.

$sf->countLines() refers to a class I made that handles small operations, in this case I was able to count the number of lines in the file uploaded by the user. I'll in the end compile my script and make one class from it. But for now I'm learning and tinkering around to see how this can be done smoothly.

$_ = NULL;
$msg = "<br />[ Your Script ".$handle->file_dst_name_body." has been uploaded ]<br />";
$file = $handle->file_dst_path.$handle->file_dst_name_body.'.'.$handle->file_dst_name_ext;
$open = fopen($file, 'r+');
$story = utf8_decode(fread($open, filesize($file)));

echo $msg.'<br />';

$sf = new TextFileReader($file);
$sf->seekLine(1); // Go to line 1

echo "Total number of lines: ".$sf->countLines()."<br />";
if(preg_match_all('/\s{68}(.*)/m', $story, $_))
{
    $unique_pages = array_unique(array_map(create_function('$a', 'return ucfirst(strtolower(trim($a)));'), $_[1]));
    echo "<span><strong>Pages: </strong>".count($unique_pages)."</span><br />";
}
$searchfor = array();
$searchfor[0] = 'INT.';
$searchfor[1] = 'EXT.';
$searchfor[2] = 'I/E.';

// get the file contents, assuming the file to be readable (and exist)
$contents = mb_convert_encoding(file_get_contents($file), "UTF-8", "auto");
// escape special characters in the query
$pattern = preg_quote($searchfor[0], '/');
// finalize the regular expression, matching the whole line
$pattern = "/^.*$pattern.*\$/m";

$list = "";
$numInt  = 0;
// search, and store all matching occurences in $matches
if(preg_match_all($pattern, $contents, $matches))
{
    $list = "<ol>";
    foreach($matches as $match) {
        $list .= "<li><div class='int'>".implode("</div></li><li><div class='int'>", $match)."</div></li>\n";
    }
    $numInt = count($matches[0]);
    $list .= "</ol>";
}else{
    $list = "No matches found";
}

// escape special characters in the query
$pattern2 = preg_quote($searchfor[1], '/');
// finalize the regular expression, matching the whole line
$pattern2 = "/^.*$pattern2.*\$/m";
$list2 = "";
$numExt  = 0;
// search, and store all matching occurences in $matches
if(preg_match_all($pattern2, $contents, $matches2))
{
    $list2 = "<ol>";
    foreach($matches2 as $match) {
        $list2 .= "<li><div class='int'>".implode("</div></li><li><div class='int'>", $match)."</div></li>";
    }
    $numExt = count($matches2[0]);
    $list2 .= "</ol>";
}else{
    $list2 = "No matches found";
}

// escape special characters in the query
$pattern3 = preg_quote($searchfor[2], '/');
// finalize the regular expression, matching the whole line
$pattern3 = "/^.*$pattern3.*\$/m";
$list3 = 0;
$numIe = 0;
// search, and store all matching occurences in $matches
if(preg_match_all($pattern3, $contents, $matches3))
{
    $list3 = "<ol>";
    foreach($matches3 as $match) {
        $list3 .= "<li><div class='int'>".implode("</div></li><li><div class='int'>", $match)."</div></li>\n";
    }
    $numIe = count($matches3[0]);
    $lis3t .= "</ol>";
}else{
    $list3 = "No matches found";
}

any help will really be helpful guys!

Upvotes: 2

Views: 247

Answers (2)

Gordon
Gordon

Reputation: 316969

Describe what the code does in simple sentences. Then make a function for each of these sentences and extract the logic into it. If you find you are repeating sentences, reuse the functions in the code.

You could also consider making the acting entities in your code into objects, e.g. use OOP. That would be a Convert procedural design to objects refactoring then:

Mechanics

  • Take each record type and turn it into a dumb data object with accessors.
  • If you have a relational database, take each table and turn it into a dumb data object.
  • Take all the procedural code and put it into a single class.
  • Take each long procedure and apply Extract Method and the related refactorings to break it down. As you break down the procedures, use Move Method to move each one to the appropriate dumb data class.
  • Continue until you’ve removed all the behavior away from the original class. If the original class was a purely procedural class, it’s very gratifying to delete it.

Upvotes: 1

John
John

Reputation: 16007

For starters, this code is written almost exactly the same way three times:

// search, and store all matching occurences in $matches
if (preg_match_all($pattern2, $contents, $matches2)) {
    $list2 = "<ol>";
    foreach($matches2 as $match) {
        $list2 .= "<li><div class='int'>".implode("</div></li><li><div class='int'>", $match)."</div></li>";
    }
    $numExt = count($matches2[0]);
    $list2 .= "</ol>";
}
else {
    $list2 = "No matches found";
}

You can pull this into its own method so you're not repeating the code (and maintaining it in all of those places separately.)

Upvotes: 2

Related Questions