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.

I've been unable to find a PHP function that will convert eol in files. This is what I have so far. It works, no errors.

Your educated opinions, thoughts, improvements, potential bugs and suggestions are welcome.

/*
    $file_name - full/path/to/file.name

    $eol         - wanted end of line: "\n", "\r", "\r\n"
        from "\n" to "\r" || "\r\n"
        from "\r\n" to "\r" || "\n"
        from "\r" to "\n"

    $len    - buffer size

    return true on success or false on fail
*/
function file_eol($file, $eol, $len=8192) {

    $ok = FALSE;
    $extension = '.my_temp';

    $hread = FALSE;
    if (is_string($file))   $hread = fopen($file, 'rb');
    if ($hread === FALSE)   e("Bad file in file_eol(): $file");
    if (!is_resource($hread) || get_resource_type($hread) !== 'stream') e("Bad hread in file_eol(): $hread");

    $hsave = fopen($file.$extension, 'wb');
    if ($hsave === FALSE)       e("Bad hsave file in file_eol(): $file.my_temp");

    switch ($eol) {
    case "\n":      $order = array("\r\n", "\r");   // from "\r\n" || "\r" -> "\n"
    break;
    case "\r":      $order = array("\r\n", "\n");   // from "\r\n" || "\n" -> "\r"
    break;
    case "\r\n":    $order = array("\n", "\r\r\n"); // from "\n" -> "\r\n"
    break;

    default:
        e("Bad eol in file_eol(): $eol");
    }


    while (($str = fread($hread, $len)) && $str !== FALSE) {    // error or eof

        $new = str_replace($order, $eol, $str);
        // if (is_string($new)) $ok = fwrite($hsave, $new);     // this ???
        if ($new !== '')    $ok = fwrite($hsave, $new);
        if ($ok === FALSE) break;   //  or  e("Error writing file: $file$extension");
    }
    $ok = $ok === FALSE ? FALSE : TRUE; // ok could be valid 0bytes
    $ok = $ok && ((bool)$str || feof($hread));          // check if fread and feof failed
    $ok = $ok && fclose($hread) && fclose($hsave);  // ALWAYS check for fclose!

    if ($ok)    $ok = rename($file.$extension, $file);  // overwrite $file, and delete $file.$extension
    // else leave both files for later inspection

return $ok;
}

P.S. copy/paste should work if you create error handler function e() that will take string inputs or just change e() to trigger_error or whatever you want.

Edit:

Must include feof()

$ok = $ok && ((bool)$str || feof($hread));          // check if fread and feof failed
share|improve this question
add comment

1 Answer 1

First of all, your function name sounds weird. The name of the function should brefly describe what it does, so, a name like convertEol would be more appropriate.

Second, I see much redundancy on your code, like this part:

if ($hread === FALSE)   e("Bad file in file_eol(): $file");
if (!is_resource($hread) || get_resource_type($hread) !== 'stream') e("Bad hread in file_eol(): $hread");

As you can see on the PHP manual of fopen function, it will return false if it is not able to open de file, otherwhise, it will ALWAYS return a resource, no matter what (except that you are doing something really crazy, like opening more files than a process is allowed to do).

So, your second verification is redundant.

Also, returning booleans in functions that do not check for some condition it too old-fashioned, too 80's. The same way, triggering uncatchable errors is almost a crime.

You should really learn how to work with Exceptions.

Here is what I would do, not necessarily the only one correct implementation:

function convertEol($fileName, $eolChar) {
    /* For windows, unix and mac.
     * IMPORTANT: for this implementation, \r\n MUST be the first element on the array
     */
    static $replaceableChars = array("\r\n", "\n", "\r"); 

    $originalFile, $tmpFile;

    try {
        $originalFile = new SplFileObject($fileName, 'r'); // open a file for reading
    } catch (RuntimeException $e) {
        throw new Exception("Unable to open file {$fileName} for reading", null, $e);
    }

    try {
        $tmpFile = new SplFileObject(uniqid() . '.tmp', 'w+'); // open or create a file for writing
    } catch (RuntimeException $e) {
        throw new Exception("Unable to create temp file", null, $e);
    }

    // iterates over the original file, line by line
    foreach ($originalFile as $line) {
        // writes the replaced string
        $tmpFile->fwrite(str_replace($replaceableChars, $eol, $line));
    }

    $tmpFile->fflush(); // makes sure file was written

    rename($tmpFile->getPathname(), $originalFile->getPathname());
}

Usage:

// Normal
convertEol('path/to/file', "\n");

// Unexistent file
convertEol('path/to/unexistent/file', "\n"); // will throw an exception

// Catching error
try {
    convertEol('path/to/unexistent/file', "\n");
} catch (Exception $e) {
    echo 'Oops, something went wrong:' . $e->getTraceAsString();
}
// ...
share|improve this answer
    
Thank you :) Questions: How do you limit memory usage? Some lines could be few megabytes long. And, in your opinion, is it ok to create e() as function e($str){throw new Exception($str);} –  CoR Feb 19 at 13:42
    
You still have to read until the end of the line to be able to change its EOL char. PHP can properly handle some few megabytes file. Don't worry about problems you might remotely have. About your function, there's a problem when you need an Exception subtype, like RuntimeException, InvalidParameterException, etc. –  Henrique Barcelos Feb 19 at 13:48
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.