Page 1 of 1
Peer review for a file_get_contents function
Posted: Fri Jan 27, 2006 9:32 am
by Roja
I'd deeply appreciate some peer review for this function.
I'm fairly certain that this meets most common expectations for a function.. no globals, distinct inputs, clear returns, etc.
I have the nagging feeling that there is something subtly non-ideal.
Feedback is appreciated.
Code: Select all
function file_get_contents($filename, $use_include_path = FALSE)
{
$data = NULL;
$file = @fopen($filename, "rb", $use_include_path);
if ($file)
{
while (!feof($file))
{
$data .= fread($file, 1024);
}
fclose($file);
return $data;
}
else
{
return FALSE;
}
}
Posted: Fri Jan 27, 2006 9:42 am
by feyd
I think there was something I read a while ago about
feof() under certain circumstances not returning false. I'm also not entirely sure it works well for remote files...
Posted: Fri Jan 27, 2006 9:50 am
by Jenk
Would the following not be better? (genuine question!)
Code: Select all
<?php
function file_get_contents($path, $use_include_path = FALSE)
{
$file = @file($path, $use_include_path);
if ($file !== FALSE) {
$filestring = implode("\r\n", $path);
} else {
return FALSE;
}
return $filestring;
}
?>
Posted: Fri Jan 27, 2006 9:54 am
by feyd
lose the \r\n (leave just an empty string) .. it may still have issues, but they would be more internal to php's core than not.
Posted: Fri Jan 27, 2006 4:05 pm
by Christopher
Maybe it's just me, but I am not wild about using @ to supress messages. I won't go so far as to call it evil as other do. I would prefer to code it something like this:
Code: Select all
function file_get_contents($filename, $use_include_path = FALSE) {
$data = FALSE;
if (file_exists($filename) && is_file($filename) && is_readable($filename)) {
$file = fopen($filename, "rb", $use_include_path);
if ($file) {
$data = fread($file, filesize($filename));
fclose($file);
}
}
return $data;
}
This code was not tested. Also I believe PHP caches the file descriptors so each call does not have to reread the file.
Posted: Fri Jan 27, 2006 11:55 pm
by josh
What is the point of reading in on KB at a time, why not just leave the option length parameter blank and read in the file all at once.
edit: ok yeah the documentation explained it, but why check for eof when fread already does that for you?
Posted: Sat Jan 28, 2006 1:02 am
by Christopher
jshpro2 wrote:edit: ok yeah the documentation explained it, but why check for eof when fread already does that for you?
I thought that you only read chunks of you were reading from a stream.
Posted: Sat Jan 28, 2006 2:22 am
by josh
Yeah, I'm guessing he wants his file_get_contents() to work with any kind of wrapper just like the 'real' version of the function