Experts Round Table Network - improve some code

seandelaney

Mentor

Offline

Posts: 119

improve some code

« on: January 25, 2006, 07:23:50 AM »

at the moment, i use this code to search for certain files in a directory on our file server.

it works fine, but the only problem, is upper and lower case filename cause problems...

example: i search for "Revs" and i get 14 results, but if i search for rev, i got none.

This is the problem. can anybody help improve it so it handles upper and lower case?

Code:

<html>
<head>
<style type="text/css">

</style>
</head>
<body>
<div id="search_form">
<?php
function show_form() {
?>
<form name="myform" action="<? echo $_SERVER['PHP_SELF']?>" method="post">
<p class="label"><strong>Pegasus UK Intranet Search Page (v1.0)</strong>
<br><br>Searching
<?php
include 'http://192.168.0.2/count.php';
?>
Documents
<br />
<br />
Enter Keywords (Search based on document titles)
<br /><br />
<input type="text" value="" name="search_term" style="width:350px;" onkeyup="this.form.submit_button.disabled=(this.value=='');"> <input type="submit" name="submit_button" value="Go!" disabled>
</form>
<?php
}

function findFileRelative($filename, &$files_found, $recurse, $extensions = array()) {
$close_factor = 2;
$path = dirname($filename);

$filebasename = basename($filename);

if (is_dir($path)) {
if ($d = opendir($path)) {
while ( ($file = readdir($d)) !== false ) {
if ($recurse && $file != "." && $file != "..") {
if (is_dir($path . "\\" . $file . "\\")) {
findFileRelative($path . "\\" . $file . "\\" . $filebasename, $files_found, $recurse, $extensions);
continue;
}
}
$last_dot = strrpos($file, ".");

if ($last_dot === false) {
$last_dot = strlen($file) - 1;
}

$file_flat = substr($file, 0, $last_dot);

$ext = substr($file, $last_dot + 1, strlen($file));
$search_word=split(" ", $filebasename);

if (count($search_word) == 0)
$search_word[0] = $filebasename;

if (in_array($ext, $extensions)) {
for($localcount=0;$localcount<count($search_word);$localcount++) {
if(substr_count($file_flat,$search_word[$localcount])>0) {
$files_found[] = $path . "\\" . $file;
}
break;
}
}
}
}
}
}

if(isset($_POST["submit_button"]) && !empty($_POST["submit_button"])) {
show_form();
$files_found = array();
findFileRelative("g:\\intranet\\" . $_POST["search_term"], $files_found, true, array("vsd","VSD","doc","DOC","pdf","PDF","xls","XLS"));
$num = count($files_found);
?>
<p class='label' style='margin-top:-10px;'>Search term : <strong><?=$_POST['search_term']?></strong>
<br/>
<br />Files's found : <strong><?=$num?></strong><br/>
<?php
if ($num > 0) {
?>
<br />Search results :
<ul style='margin-top:0px;' class='label'>
<?php
for ($i = 0;$i < count($files_found);$i++) {
$files_found[$i] = preg_replace("/g:\\\\intranet\\\\/i", "", $files_found[$i]);
$files_found[$i] = preg_replace("/\\\\/", "/", $files_found[$i]);

$url = "intranet\\".$files_found[$i];
$user_url = "http://192.168.0.2/intranet/".$files_found[$i];
$pieces = explode("/", $user_url);
$count = count($pieces);
$pieces[$count-1];
?>
<li style='margin-top:3px;margin-bottom:3px;'><a href='<?=$url;?>' target='_blank'><?=$files_found[$i];?></a></li>
<?php
}
?>
</ul>
<?php
} else {
?>
<br>Search results : <strong>none</strong><br><br>Sorry, your keyword(s) did not match any titles.<br><br>Please try again and choose your keyword(s) carefully.
<?php
}
?>
</p>
<?php
} else {
show_form();
}
?>
</div>
</body>
</html>


	Logged

Sean
Mentor Written Articles: http://www.expertsrt.com/articles/index.shtml

Srirangan

Moderator

Offline

Posts: 52

improve some code

« Reply #1 on: January 25, 2006, 08:42:19 AM »

Convert both the query and the search parse results to either Upper case or Lower case, and then compare. I haven't gone through your code, but this method could be used in most cases.


	Logged

I rant therefore I am!

seandelaney

Mentor

Offline

Posts: 119

improve some code

« Reply #2 on: January 25, 2006, 08:47:42 AM »

where do i do the comparing though?


	Logged

Sean
Mentor Written Articles: http://www.expertsrt.com/articles/index.shtml

Zyloch

Site Builder

Offline

Offline

Posts: 8

improve some code

« Reply #3 on: January 25, 2006, 11:34:48 AM »

You may do the comparing however you normally do it. You can include an extra boolean parameter in your findFileRelative() function for case sensitivity. If it is case insensitive, you can use strtolower() (or strtoupper() if you choose to convert everything to uppercase) to change the search term and all the extensions in the array to lowercase (or just make sure all the extensions are in lowercase--I'm not too sure if mixed-case extensions make a difference, but just in case).

If all the files in your directory are all lowercase, or uppercase, or has any other pattern in cases, you only need to compare as usual, but otherwise, everytime you find a file, you need to convert the case to lowercase (or uppercase if you chose to make the search term like that).

HOWEVER, PHP contains many case-insensitive functions as well. Of them, there is stripos() which checks for the position of a string in another string, case insensitive, and returns boolean false on failure.

Therefore, we can use:

Code:

if (false !== stripos($file_flat, $search_word[$localcount]))
{
$files_found[] = $path . '\\' . $file;
break;
}

Notice that the break; statement is placed within the if clause. We only want the search loop to end after we've found the file! Not after only checking the first string in in the array (according to your program logic).

I haven't read through your code completely and checked it thoroughly so I do not have any other suggestions to make than an answer to your question (Java class in school is a joke :-D), but either me or another mentor will probably check over the entire code to make suggestions for improvement (if there are any).


	Logged

Ted

Zyloch

Site Builder

Offline

Offline

Posts: 8

improve some code

« Reply #4 on: January 25, 2006, 04:10:35 PM »

Ok, I'm back :-D In the spirit of mentoring, I'll try to go over all that I can go over in terms of improving the code. Some may even be considered as trivial, but even trivial knowledge is good knowledge IMHO, so I'll just go through the code from the top.

First of all, ignoring the lack of a doctype, I am not quite sure that the overflow-y style is necessary. I do not see anything in your code that would prompt it, and I'm confident that the default value for overflow-y (for IE) as it applies to the body tag is auto, not visible. [Correct me if I'm wrong]

Now, the 'visited' pseudoclass is spelled wrong (a:visted, should be a:visited), probably just a simple typo that can be fixed. In order for the a:active to be effective, it should have to come after the a:hover, although certain browsers may render it no matter what. For information on the order of the anchor pseudoclasses, check http://www.w3schools.com/css/css_pseudo_classes.asp

Ok, now I'm at your PHP function, show_form() to be precise. There's not much wrong with it--an only suggestion is to keep to one style and use it (for instance, you have both <br> and <br />), but that's just coding style. As for the submit button, I am not too sure whether the space within the input tag is just a typo or because of the code block, but it should be deleted in any case.

Another possible suggestion, if you are worried about file size, is to put all your functions, or all your separated HTML code in separate files. If it is all in one file as in your case, the entire file will need to load regardless of whether the form is to be used or not.

If instead, the form, or function show_form() is included in another file that is included with include_once() when you test for form submit, it could drastically shorten load times. (Same for your findFileRelative() function). Thus, something like:

Code:

<?php
if ( !empty($_POST['submit_button'] )
{
include_once('finding_files_func.php');
}
else
{
include_once('my_form.php');
}
?>

Although in your case, the form is shown no matter what, perhaps something like:

Code:

<?php
include_once('my_form.php');
if ( !empty($_POST['submit_button']) )
{
include_once('my_form.php');
//rest of code
}
?>

You do not even need show_form() if the complete form is in myform.php. Notice also that I did not need to check isset($_POST['submit_button']) because !empty() will check isset() as well.

Well, you're probably bored silly with half of my rant, especially my points about CSS in the beginning that you may already know ;-), so I'll get to the main part, the findFileRelative() function.

To start off, I see no use for the $close_factor variable, as it is only declared, not used. Perhaps this is something you were saving for code cleanup?

Thus, this line:

Code:

$close_factor = 2;

is unnecessary.

In addition, there is one line of code, that would be better off moved to the outside of the inner while loop. In particular,

Code:

$search_word=split(" ", $filebasename);

would better be served if moved to right after

Code:

$filebasename = basename($filename);

or even in any of the if clauses, as long as it is above the while loop. This saves the code from being needless executed over and over again for every file in the directory (and possibly a lot more if it's recursive) when it only needs to be done once. Another suggestion is to use explode() rather than split() as you do not require any complex regular expressions (in which case preg_split() is often faster).

Finally, this code:

Code:

if (count($search_word) == 0)
$search_word[0] = $filebasename;

can be safely omitted as explode() [and split() for that matter] will automatically return an array in which the first member is the string if there is no breaks.

Now, this code:

Code:

$ext = substr($file, $last_dot + 1, strlen($file));
//... code
if (in_array($ext, $extensions)) {

This makes the default setting of $extensions = array() in the function header completely useless. This is because in order for any file to be tested, the '' element must be present in the $extensions array. Notice $ext will be '' if there is no dot, and that array() is not the same as array('').

Thus, unless you want it that way, I would propose having something more like:

Code:

$last_dot = strrpos($file, '.');
if (false !== $last_dot)
{
//we have an extension
$ext = substr($file, $last_dot + 1, strlen($file));
if (false === in_array($ext, $extensions))
{
continue;
}
else
{
$file_flat = substr($file, 0, $last_dot);
}
}
else
{
$file_flat = $file;
}

We're almost done! The only thing left is to check your for loop. It is mostly alright. The code with a few improvements would be:

Code:

for($localcount = 0; $localcount < $num_search_terms; $localcount++)
{
if( false !== stripos($file_flat, $search_word[$localcount]) )
{
$files_found[] = $path . '\\' . $file;
break;
}
}

I moved the break; statement to the inside of the if clause so the for loop does not prematurely break.

IMPORTANT: See the $num_search_terms variable? It doesn't exist. You need to add this code right below where I told you to move the split() in your program. Thus, it would be:

Code:

$search_word = explode(' ', $filebasename);
$num_search_terms = count($search_word);

This is because the count() function takes time, not a lot, but time nevertheless. It all adds up eventually. Especially if it is within the second part of the for loop. This means every execution of the loop, count($search_word) will be evaluated again!. If the while loop runs 100 times for 100 files, and the for loop runs 5 times for 5 search terms, that 500 extra function calls to count()!

Moving the count() function out of the while loop speeds up the function :-)

Therefore, the final function with my suggestions would be:

Code:

The only other change I've made is a $path_separator variable to make changing the separator much easier (if you want it with / instead of \ for instance).

EDIT: I rechecked the code, fixed a few minor errors, and also realized that the changed code for extensions means that folders will also be listed. I changed the if clauses around so that folders will not be listed even if they match. This is because if the file is a folder, the is_dir check takes care of it and there is a continue; statement inside that clause.

If you want to make an option for both case sensitive and case insensitive search, simply add another parameter in your function header, then in the for loop, you can use an if clause to decide whether to use stripos() or the case-sensitive strpos().

Hopefully, I've helped you with your code and knowledge :-D Signing out for now :-D

Ted


	Logged

Ted

seandelaney

Mentor

Offline

Posts: 119

improve some code

« Reply #5 on: January 26, 2006, 02:41:52 AM »

Quote

If you want to make an option for both case sensitive and case insensitive search, simply add another parameter in your function header, then in the for loop, you can use an if clause to decide whether to use stripos() or the case-sensitive strpos().

Where do i actually add this check?

I added it to here:

Code:

for ($localcount = 0; $localcount < $num_search_terms; $localcount++)
{

$check = ($cases == 'sensitive') ? strpos($file_flat, $search_word[$localcount]) : stripos($file_flat, $search_word[$localcount]);

if ( false !== $check)
{
$files_found[] = $path . $path_separator . $file;
break;
}
}

but now i get no results found...

in my function header i have this:

Code:

function findFileRelative($filename, &$files_found, $recurse, $cases, $extensions = array())

Am i write in what you meant?


	Logged

Sean
Mentor Written Articles: http://www.expertsrt.com/articles/index.shtml

Zyloch

Site Builder

Offline

Offline

Posts: 8

improve some code

« Reply #6 on: January 26, 2006, 04:43:57 AM »

I cannot find anything wrong with that, although $cases would probably do better as a boolean with values true and false. Thus, you'd have:

Code:

$check = (true === $cases) ? strpos($file_flat, $search_word[$localcount]) : stripos($file_flat, $search_word[$localcount]);

in the exact same place you put it. How are you calling your function?


	Logged

Ted

seandelaney

Mentor

Offline

Posts: 119

improve some code

« Reply #7 on: January 26, 2006, 05:42:21 AM »

its ok, i got it working...


	Logged

Sean
Mentor Written Articles: http://www.expertsrt.com/articles/index.shtml

Anonymous

Guest

improve some code

« Reply #8 on: January 30, 2006, 06:03:43 AM »

Hi.

Even though I am ZCE, I am still getting to grips with PHP's SPL.

The following class should provide you with a start point for simplifying the entire findFileRelative function.

Code:

<?php
class DepthLimitingIterator extends RecursiveIteratorIterator
{
/** Constructs a RecursiveIteratorIterator
*
* @param o_Iterator RecursiveIterator to iterate
* @param i_MaxDepth new maximum allowed depth or -1 for any depth
* Use 1 for no recursion
* @param i_Mode Operation mode (one of):
* - LEAVES_ONLY only show leaves
* - SELF_FIRST show parents prior to their childs
* - CHILD_FIRST show all children prior to their parent
* @param i_Flags Control flags, zero or any combination of the following
* (since PHP 5.1).
* - CATCH_GET_CHILD which catches exceptions during
* getChildren() calls and simply jumps to the next
* element.
*/
function __construct(RecursiveIterator $o_Iterator, $i_MaxDepth = -1, $i_Mode = self::LEAVES_ONLY, $i_Flags = 0)
{
// Call the parent constructor.
parent::__construct($o_Iterator, $i_Mode, $i_Flags);

// Assign the required depth
$this->setMaxDepth($i_MaxDepth);
}
}

class FileExtensionFilter extends FilterIterator
{
private
$as_Extensions;

/**
* Constructs a filter around an iterator whose elemnts are strings.
* If the given iterator is of type spl_sequence then its rewind()
* method is called.
*
* @param o_Iterator Object that implements at least spl_forward
* @param as_Extensions Array of allowable extensions
*/
function __construct(Iterator $o_Iterator, array $as_Extensions = NULL)
{
// Empty the extensions array.
$this->as_Extensions = array();

// Call the parent constructor
parent::__construct($o_Iterator);

// Populate the extensions array with lower case variants of the supplied extensions array.
if (is_array($as_Extensions))
{
foreach($as_Extensions as $s_Extension)
{
if (!in_array(strtolower($s_Extension), $this->as_Extensions))
{
$this->as_Extensions[] = strtolower($s_Extension);
}
}
}
}

/**
* Accept function to decide whether an element of the inner iterator
* should be accessible through the Filteriterator.
*
* @return whether or not to expose the current element of the inner
* iterator.
*/
function accept()
{
// If the extension of the current file is in the list of valid extensions then allow this file through.
return (in_array(strtolower(pathinfo($this->getPathname(), PATHINFO_EXTENSION)), $this->as_Extensions));
}
}

define('NO_RECURSION', 0);
define('ALLOW_RECURSION', -1);
define('INITIAL_FILE_LOCATION', 'g:/intranet/');

// NOTE: The uppercase variants are no longer required as the class will deal with this appropriately.
$as_ValidExtensions = array
(
'vsd',
'VSD',
'doc',
'DOC',
'pdf',
'PDF',
'xls',
'XLS',
);

foreach(new FileExtensionFilter(new DepthLimitingIterator(new RecursiveDirectoryIterator(INITIAL_FILE_LOCATION), NO_RECURSION), $as_ValidExtensions) as $s_Filename => $o_FILE)
{
echo "This file is valid $s_Filename\n";
}
?>


	Logged

Anonymous

Guest

improve some code

« Reply #9 on: January 30, 2006, 06:09:26 AM »

Take away all the comments and the initialising code, you effectively end up with 3 lines of REAL code.

1 - Assigning the maximum depth to look through (either -1 for all levels or 1 for just the 1 level).
2 - Determining if the extension is appropriate
3 - The foreach() line.

All of this code is extendable.

And is modular.

And is reusable.

You could for example create another filter to deal with the name containing specific words and wrap that around the foreach's array_expression (as it is called in the PHP manual).

I hope this is of some use and will lead to a leaner program and allow for more reusable code.


	Logged

seandelaney

Mentor

Offline

Posts: 119

improve some code

« Reply #10 on: January 30, 2006, 07:08:44 AM »

oh man!

thank you so much for your help. wasnt expecteding that - i though nobody else would have commented...

it looks great, but how do i intergrate it with my code?


	Logged

Sean
Mentor Written Articles: http://www.expertsrt.com/articles/index.shtml

Anonymous

Guest

improve some code

« Reply #11 on: January 30, 2006, 07:24:53 AM »

If you look at

Code:

foreach(new FileExtensionFilter(new DepthLimitingIterator(new RecursiveDirectoryIterator(INITIAL_FILE_LOCATION), NO_RECURSION), $as_ValidExtensions) as $s_Filename => $o_FILE)
{
echo "This file is valid $s_Filename\n";
}

this is effectively going to provide you with a filename which has one of the required extensions.

What do you do next?

Does that code behave like a filter (i.e. rejecting some of the names?)

If so, create a new filter class and wrap it ...

Code:

<?php
class DepthLimitingIterator extends RecursiveIteratorIterator
{
function __construct(RecursiveIterator $o_Iterator, $i_MaxDepth = -1, $i_Mode = self::LEAVES_ONLY, $i_Flags = 0)
{
parent::__construct($o_Iterator, $i_Mode, $i_Flags);
$this->setMaxDepth($i_MaxDepth);
}
}

class FileExtensionFilter extends FilterIterator
{
private
$as_Extensions;

function __construct(Iterator $o_Iterator, array $as_Extensions = NULL)
{
$this->as_Extensions = array();

parent::__construct($o_Iterator);

if (is_array($as_Extensions))
{
foreach($as_Extensions as $s_Extension)
{
if (!in_array(strtolower($s_Extension), $this->as_Extensions))
{
$this->as_Extensions[] = strtolower($s_Extension);
}
}
}
}

function accept()
{
return (in_array(strtolower(pathinfo($this->getPathname(), PATHINFO_EXTENSION)), $this->as_Extensions));
}
}

class FileNameFilter extends FilterIterator
{
private
$as_SearchWords,
$i_SearchWordCount;

function __construct(Iterator $o_Iterator, array $as_SearchWords = NULL)
{
$this->as_SearchWords = array();

parent::__construct($o_Iterator);

if (is_array($as_SearchWords))
{
foreach($as_SearchWords as $s_SearchWord)
{
if (!in_array(strtolower($s_SearchWord), $this->as_SearchWords))
{
$this->as_SearchWords[] = strtolower($s_SearchWord);
}
}
}
$this->i_SearchWordCount = count($this->as_SearchWords);
}

function accept()
{
$as_SplitFilename = explode(' ', pathinfo($this->getPathname(), PATHINFO_BASENAME));
$i_WordsFoundCount = 0;
foreach($as_SplitFilename as $s_FilenameWord)
{
if (in_array(strtolower($s_FilenameWord), $this->as_SearchWords))
{
++$i_WordsFoundCount;
}
}
return ($this->i_SearchWordCount == $i_WordsFoundCount);
}
}

define('NO_RECURSION', 0);
define('ALLOW_RECURSION', -1);
define('INITIAL_FILE_LOCATION', 'C:/');

// NOTE: The uppercase variants are no longer required as the class will deal with this appropriately.
$as_ValidExtensions = array
(
'vsd',
'VSD',
'doc',
'DOC',
'pdf',
'PDF',
'xls',
'XLS',
);

$as_SearchWords = array
(
// Populate this array with your form's results.
);

foreach
(
new FileNameFilter
(
new FileExtensionFilter
(
new DepthLimitingIterator
(
new RecursiveDirectoryIterator
(
INITIAL_FILE_LOCATION
),
NO_RECURSION
),
$as_ValidExtensions
),
$as_SearchWords
)
as $s_Filename => $o_FILE)
{
// Process each valid file here.
echo "This file is valid $s_Filename\n";
}?>

The classes can be hived off into a require_once() file. The main application's responsibility is to get the data from the user, pass it through the foreach to get the valid files and then process them.


	Logged

Pages: [1]

« previous next »