Scrape HTML into separate files for each chapter

Question

I have written a program to read HTML file, find particular text, write text from that particular text to next particular text in a newly opened file. I am interested in code review. I am very new PHP.

<?php

$FILE = "testing.html";
$count = 0;
$directoryName = explode('.',$FILE)[0];
$TAG = "/x-berschrift-1--nur-f-r-Header-/";

function readingFile($FILE)
{
    //Creates a directory on the file name to store segmeted chapters from $FILE
    global $directoryName;
    mkdir($directoryName);
    $file_handle = fopen($FILE, "r");
    chdir($directoryName);
    $matchTag = False;
    while (!feof($file_handle))
     {
           $line = fgets($file_handle);
           if(checkingMatchTag($line))
           {
                if($matchTag){
                    closeWriteFile($writeHandle);
                    incrementCounter();
                }
                else
                {
                    $matchTag = True;
                }
                $writeHandle = openWriteFile();
                segmentContent($writeHandle, $line);
           }        
           elseif($matchTag)
           {
               segmentContent($writeHandle, $line); 
           }
     } 
}

function checkingMatchTag($line)
{
    global $TAG;
    if(preg_match($TAG, $line))
    {
        return True;
    }
    return False;
}

function openWriteFile()
{
    global $directoryName, $count, $FILE;
    $writingFile = fopen($directoryName.$count.'.'.explode('.',$FILE)[1], 'w');
    return $writingFile;
}

function segmentContent($writeFileHandle, $line)
{
   fwrite($writeFileHandle, $line);
}


function closeWriteFile($file)
{
    fclose($file);
}

function incrementCounter()
{
    global $count;
    $count++;
}


readingFile($FILE);

Answer 1

Don't use globals in this case

To start off, global variables should generally be avoided. Often times configurations will use global constants, but using global variables can have implications to the namespace. See pacmaninbw's answer for a little more explanation on that.

With that said, if you wanted to keep the values global please use constants.

define('FILE', "testing.html");
define('TAG', "/x-berschrift-1--nur-f-r-Header-/");
define('DIRECTORY', "testing");

Define global constants like this at the beginning of your file.

---

Simplify (and remove) your functions

Next up are your functions:

function checkingMatchTag($line)
{
    return preg_match(TAG, $line);
}

function openWriteFile($directory, $count, $extension)
{
    return fopen($directory . $count . '.' . $extension, 'w');
}

function closeWriteFile($file)
{
    fclose($file);
}

function segmentContent($writeFileHandle, $line)
{
    fwrite($writeFileHandle, $line);
}

As you can see, I simplified your functions for you. They perform the same, but have less clutter involved. In checkingMatchTag I left in the call to TAG in to show you how to call global constants. Anyway, the point is each of these functions can be reduced to one simple line. So why are you making a ton of functions for one line of basic code? I would advise you get rid of each of these functions and simply use the line of code.

---

Misc changes

Make sure the directory is not there by using this code:

if (!file_exists(DIRECTORY)) {
    mkdir(DIRECTORY);
}

Use the same naming convention for everything. Look at your perimeters. Look at your spacing. Look at your {}. There are differences in all of them.

Answer 2

This would be pretty good code in PHP4, but PHP has become more object oriented.

Implement this code as a class. A class gets you multiple benefits

1. A class is reusable.
2. A class can be added to a library more easily.
3. A class will reduce the number of global variables you have.

Each of your global variables could be private to the class.

Global variables are bad because they impact the namespace of all the modules that share this code.

$directoryName = explode('.', $FILE)[0];

May not work as expected, a file name especially a Fully qualified Name may have multiple '.' in it. How would your explode work on http::/andDomain.com/Test.File.html

Since you only call incrementCount() in one place, replace that call with the actual code and make count local to the function. Pass $count as an ar guement to openWriteFile().

Initialize what are currently global variables in a constructor for the class.

class MyScraper
{
    protected $FILE;
    protected $count;
    protected $directoryName
    protected $TAG;

    function __construct($fileName, $tag);
    {
        $this->FILE = $fileName;
        $this->TAG = $tag; 
        $this->directoryName = explode('.',$FILE)[0];
        $this->count = 0;
    }

    function readingFile() 
    {
        mkdir($this->directoryName);
        $file_handle = fopen($this->FILE, "r");
        ...
    }

    ...
}

scraper = new MyScraper("testing.html", "/x-berschrift-1--nur-f-r-Header-/");

scraper->readingFile();

Answer 3

Consider changing the name of readFile to something like segmentFile. Because it doesn't just read the file. I wouldn't expect something named readFile to write new files.

function closeWriteFile($file)
{
    fclose($file);
}

What's the purpose of this? You're just rewriting a function call as...a function call. If this were in a class and $file was an object field, then it would make more sense. But as it is, there doesn't seem any reason why closeWriteFile($file) is any better than fclose($file).

The same issue with segmentContent. You are abstracting something that doesn't need it. Just say fwrite($writeHandle, $line);. No need for additional complexity. That just hides what you are actually doing, making the code harder to read.

The other functions make a little more sense, but not that much. Generally, you want to make functions from a sequence of operations, not just as aliases for a single operation. For example, you could take

$directoryName = explode('.',$FILE)[0];

and

    //Creates a directory on the file name to store segmeted chapters from $FILE
    global $directoryName;
    mkdir($directoryName);
    $file_handle = fopen($FILE, "r");
    chdir($directoryName);

and rewrite this as

    $file_handle = fopen($FILE, "r");

    global $directoryName;
    $directoryName = explode('.', $FILE)[0];
    changeToDirectory($directoryName);

with

function changeToDirectory($directoryName)
{
    if (! file_exists($directoryName))
    {
        mkdir($directoryName);
    }
    chdir($directoryName);

    return $directoryName;
}

I put the global line in purely for consistency with your original code. If the original code is all there is, that line is unnecessary. You only use $directoryName in this scope and the calling method. The return handles the calling method. If you have additional code that relies on $directoryName being set, then I think that @pacmaninbw's answer is right: you should use a class rather than a series of functions with global variables to connect them.

Even this function isn't really necessary. The code is actually shorter without it. But it does abstract out a feature that you could reuse, even if you don't now.

    $matchTag = False;
    while (!feof($file_handle))
     {
           $line = fgets($file_handle);
           if(checkingMatchTag($line))
           {
                if($matchTag){
                    closeWriteFile($writeHandle);
                    incrementCounter();
                }
                else
                {
                    $matchTag = True;
                }
                $writeHandle = openWriteFile();
                segmentContent($writeHandle, $line);
           }        
           elseif($matchTag)
           {
               segmentContent($writeHandle, $line); 
           }
     }

You don't need $matchTag. Because PHP is weakly typed, you can instead say

     global $TAG;

     $count = 0;
     $extension = '.' . explode('.', $FILE)[1];
     $writeHandle = false;

     while (!feof($file_handle))
     {
           $line = fgets($file_handle);
           if (preg_match($TAG, $line))
           {
                if ($writeHandle)
                {
                    fclose($writeHandle);
                    $count++;
                }

                $writeHandle = fopen($directoryName . $count . $extension, 'w');
           }

           if ($writeHandle)
           {
               fwrite($writeHandle, $line); 
           }
     }

Again, if you don't need to use $count other than the code you posted, you don't need it to be global. If you do use it elsewhere, then a class would handle it in a more reusable way.

The $TAG global might better be a function parameter, so you'd say something like segmentFile($FILE, '/x-berschrift-1--nur-f-r-Header-/');.

In PHP, double quotes allow variable interpolation and single quotes don't. So I tend to use single quotes for any string where I don't have to use double quotes.

This also gets rid of the helper functions. Other than a little setup, it doesn't increase the length of this section of code. Calculating $extension beforehand saves recalculating it every time you see a match.

You can actually get rid of the if statements as well if you like.

     while (!feof($file_handle))
     {
           $line = fgets($file_handle);
           if (preg_match($TAG, $line))
           {
                $writeHandle = fopen($directoryName . $count . $extension, 'w');
                fwrite($writeHandle, $line);
                break;
           }
     }

     while (!feof($file_handle))
     {
           $line = fgets($file_handle);
           if (preg_match($TAG, $line))
           {
                fclose($writeHandle);
                $count++;

                $writeHandle = fopen($directoryName . $count . '.' . $extension, 'w');
           }

           fwrite($writeHandle, $line); 
     }

There is some duplication of code in that but it saves checking that it's not the first match on each iteration.