1
\$\begingroup\$

I have a modelScan.php file, as you can see,

I have codes that repeated within the function/method. How can I make this code more readable?

<?php
/**
 * Class for Scanning files
 */
class modelScan
{
    /**
     * Get the total size of file in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getBytesTotal($sPath, $iTotalCount)
    {
        $sPath = realpath($sPath);
        if($sPath!==false && $sPath!='' && file_exists($sPath)){
            foreach(new RecursiveIteratorIterator(new RecursiveDirectoryIterator($sPath, FilesystemIterator::SKIP_DOTS)) as $oObject){
                $iTotalCount += $oObject->getSize();
            }
        }
        return $iTotalCount;
    }

    /**
     * Get the total files in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getFilesTotal($sPath, $iTotalCount)
    {
        $ite = new RecursiveDirectoryIterator($sPath);
        foreach (new RecursiveIteratorIterator($ite) as $filename=>$cur) {
            if(is_file($cur)){
                $iTotalCount++;   
            }           
        }
        return $iTotalCount;
    }

    /**
     * Get the total Class in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getClassTotal($sPath, $iTotalCount)
    {
        $ite = new RecursiveDirectoryIterator($sPath);
        foreach (new RecursiveIteratorIterator($ite) as $filename=>$cur) {          
            if(is_file($cur)){
                $fileToString = file_get_contents($cur);
                $token = token_get_all($fileToString);
                $tokenCount = count($token);

                //Class Count
                $sPathInfo = pathinfo($cur);
                if ($sPathInfo['extension'] === 'php') {
                    for ($i = 2; $i < $tokenCount; $i++) {
                        if ($token[$i-2][0] === T_CLASS && $token[$i-1][0] === T_WHITESPACE && $token[$i][0] === T_STRING ) {
                        $iTotalCount++;
                        }
                    }
                } else {
                    error_reporting(E_ALL & ~E_NOTICE);
                }               
            }
        }
        return $iTotalCount;
    }

    /**
     * Get the total Method in the directory
     * @param string $sPath and integer $iTotalCount
     */
    public function getMethodTotal($sPath, $iTotalCount)
    {
        $ite = new RecursiveDirectoryIterator($sPath);
        foreach (new RecursiveIteratorIterator($ite) as $filename=>$cur) {         
            if(is_file($cur)){ 
                $fileToString = file_get_contents($cur);
                $token = token_get_all($fileToString);
                $tokenCount = count($token);

                //Method Count 
                $sPathInfo = pathinfo($cur);
                if ($sPathInfo['extension'] === 'php') {
                    for ($i = 2; $i < $tokenCount; $i++) {
                        if ($token[$i-2][0] === T_FUNCTION) {
                                $iTotalCount++;
                        }
                    }
                } else {
                    error_reporting(E_ALL & ~E_NOTICE);
                }               
            }
        }       
        return $iTotalCount;
    }


}
\$\endgroup\$
3
\$\begingroup\$

Welcome to Code Review. Your code is understandable, so we can, more or less, figure out what it does, but it is always nice for reviewers to hear you say what you use a piece of code for, what its purpose is, what the code itself specifically does, and to have some usage examples. This also helps you to reflect on the choices you made.

I couldn't get your getClassTotal() and getMethodTotal() to work. They assume that all the files are PHP files, and in my test directory they are not. You should have checked the PHP file extension before tokenizing the file content.

From what I can see this looks like a class that wraps four independent function. That only doesn't justify the existence of this class. Is there nothing common to these functions? I think we can agree that the RecursiveDirectoryIterator and RecursiveIteratorIterator are present in all four, so I made that part of the class.

There are also some simplifications I added in the file and bytes counter.

Using the build-in PHP tokenizer was a good idea, but clearly the way that classes and methods are counted is very similar. So I created a new method which will count either. I don't see the point of checking for a class name, so I left that bit out.

In your code a counter is the second argument to all four method. I fail to see why this is a good thing. I dropped it.

This gives me the following code:

<?php

class DirectoryScanner
{
    private $files;

    public function __construct($path)
    {
        $dirIterator = new RecursiveDirectoryIterator($path, FilesystemIterator::SKIP_DOTS);
        $this->files = new RecursiveIteratorIterator($dirIterator);
    }

    public function getFileCount()
    {
        return iterator_count($this->files);
    }

    public function getBytesUsed()
    {
        $bytes = 0;
        foreach ($this->files as $file) {
            $bytes += $file->getSize();
        }
        return $bytes;
    }

    private function getPhpTokenCount($token)
    {
        $counter = 0;
        foreach ($this->files as $file) {
            if ($file->getExtension() == 'php') {
                $contents = file_get_contents($file);
                $tokens   = token_get_all($contents);
                $values   = array_count_values(array_column($tokens, 0));
                $counter += isset($values[$token]) ? $values[$token] : 0;
            }
        }
        return $counter;
    }


    public function getPhpClassCount()
    {
        return $this->getPhpTokenCount(T_CLASS);
    }

    public function getPhpFunctionCount()
    {
        return $this->getPhpTokenCount(T_FUNCTION);
    }
}

?>

I left out all the comments to save some space. This code can be tested like this:

$path = $_SERVER['DOCUMENT_ROOT'];
$test = new DirectoryScanner($path);

echo 'getFileCount: ' . $test->getFileCount() . '<br>';
echo 'getBytesUsed: ' . $test->getBytesUsed() . '<br>';
echo 'getPhpClassCount: ' . $test->getPhpClassCount() . '<br>';
echo 'getPhpFunctionCount: ' . $test->getPhpFunctionCount() . '<br>';

In my code I take advantage of what a class is: An piece of code in which you share fields and methods in a local scope. I made $files and getPhpTokenCount() private intentionally to stress this point.

I think the first two methods have little in common with the last three. So I would split this up in two classes, like so:

class DirectoryScanner
{
    protected $files;

    public function __construct($path)
    {
        $dirIterator = new RecursiveDirectoryIterator($path, FilesystemIterator::SKIP_DOTS);
        $this->files = new RecursiveIteratorIterator($dirIterator);
    }

    public function getFileCount()
    {
        return iterator_count($this->files);
    }

    public function getBytesUsed()
    {
        $bytes = 0;
        foreach ($this->files as $file) {
            $bytes += $file->getSize();
        }
        return $bytes;
    }
}


class PhpFilesScanner extends DirectoryScanner
{

    public function getPhpTokenCount($token)
    {
        $counter = 0;
        foreach ($this->files as $file) {
            if ($file->getExtension() == 'php') {
                $contents = file_get_contents($file);
                $tokens   = token_get_all($contents);
                $values   = array_count_values(array_column($tokens, 0));
                $counter += isset($values[$token]) ? $values[$token] : 0;
            }
        }
        return $counter;
    }


    public function getPhpClassCount()
    {
        return $this->getPhpTokenCount(T_CLASS);
    }

    public function getPhpFunctionCount()
    {
        return $this->getPhpTokenCount(T_FUNCTION);
    }
}

?>

It just makes more sense this way. Note that PhpFilesScanner does rely on DirectoryScanner for the iterator functionality.

I think, depending on your usage, that result buffering would be a nice addition to these classes. For instance, by once buffering alle the token counts, the PhpFilesScanner class can return multiple requests for counts of certain tokens much quicker. Especially if you expand it to include more types of tokens. Buffering would make the class look something like this:

class PhpFileScanner extends DirectoryScanner
{
    private $tokenCounts = [];

    public function __construct($path)
    {
        parent::__construct($path);
        foreach ($this->files as $file) {
            if ($file->getExtension() == 'php') {
                $contents = file_get_contents($file);
                $tokens   = token_get_all($contents);
                $counts   = array_count_values(array_column($tokens, 0));
                foreach ($counts as $token => $count) {
                    if (!isset($this->tokenCounts[$token])) {
                        $this->tokenCounts[$token] = 0;
                    }
                    $this->tokenCounts[$token] += $count;
                }
            }
        }
    }

    public function getPhpClassCount()
    {
        return $this->tokenCounts[T_CLASS] ?? 0;
    }

    public function getPhpFunctionCount()
    {
        return $this->tokenCounts[T_FUNCTION] ?? 0;
    }
}

The ?? is the new Null coalescing operator of PHP 7.

\$\endgroup\$
  • \$\begingroup\$ +1 simply for introducing me to the ?? operator! Thanks! \$\endgroup\$ – Geoffrey May 2 at 23:04
  • \$\begingroup\$ +1 Thanks for your time . \$\endgroup\$ – jaybee satulan May 3 at 0:00
  • \$\begingroup\$ I will mark it as acceptable answer. \$\endgroup\$ – jaybee satulan May 3 at 8:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.