2
\$\begingroup\$

I just wanted to know if this looks like a good way of reading a config file. Is this code good? Is it clean and easy to follow?

<?
    /* class to parse a txt config file
     *
     * Boolean like string values (on/off, yes/no, true/false) will return true/false
     * Numberic string values will return real numeric values
     *
     * Ingore error on input.
     *
     * useage: $cp = new config_parse();
     *         $config_parse->parse_file($input_file);
     *
     * $input_file - the file to be parsed. If a full path is not specified, will assume the file is in the same path as the script calling this class.
     * returns an array of config values in their correct type or false if an error is encountered.
     *
     */

    class config_parse {

        var $config_array;
        var $config_file;
        var $comment_marker = "#";
        var $bool_true_array = array("on", "yes", "true");
        var $bool_false_array = array("off", "no", "false");

        function config_parse() {

            $this->config_array = array();
            $this->config_file = "";

        }

        // parse the config file into the config array
        public function parse_file($input_file) {

            if ($input_file=="") {
                return false;
            }

            // locate the config file
            if (file_exists($input_file)) {
                $this->config_file = $input_file;
            } elseif (file_exists(dirname(__FILE__) . $input_file)) {
                $this->config_file = dirname(__FILE__) . $input_file;
            } else {
                return false;
            }

            $this->do_parse();
            return $this->config_array;

        }

        // read the lines of the file
        private function do_parse() {
            $handle = fopen($this->config_file, "r");
            if ($handle) {
                while (($line = fgets($handle)) !== false) {
                    $this->process_config_line($line);
                }
                fclose($handle);
            } else {
                return false;
            } 
        }

        // remove blank lines and comments
        private function process_config_line($line) {
            $line = trim($line);

            // blank
            if ($line=="") {
                return;

            // comment
            } elseif (substr($line,0,1)==$this->comment_marker) {
                return;

            // actual value
            } else {
                $this->process_name_value($line);
            }
        }

        // process a line with an actual name/value pair in it.
        private function process_name_value($line) {

            $line_split = explode("=",$line);
            $name = trim($line_split[0]);
            $value = trim($line_split[1]);

            $this->config_array[$name]=$this->process_value($value);
        }

        private function process_value($value) {

            if (in_array($value,$this->bool_true_array)) return true;
            if (in_array($value,$this->bool_false_array)) return false;
            if (is_numeric($value)) return $value+0;
            return $value;

        }
    }
    ?>
\$\endgroup\$
2
\$\begingroup\$

This code looks pretty good, but you seem to have largely reinvented parse_ini_file($filename, false, INI_SCANNER_TYPED). All you need to add is the support for the search path.

do_parse() is inconsistent about returning a status code, and parse_file() fails to check it.

You should call explode("=", $line, 2), so that any = character in the value will be interpreted literally.

\$\endgroup\$
1
\$\begingroup\$

The code itself is mostly ok my comments focus mostly on stylistic changes, as your code seems to be based on PHP 4 standards. Unless you absolutely have to use PHP 4, I have the following recommendations to bring the code in line with modern practices.

  1. Consider adopting psr-1 + psr-2

Much of the php community now use http://www.php-fig.org/psr/psr-1/ and http://www.php-fig.org/psr/psr-2/ as guidelines for code layout. Some of the rules are stylistic, others such as no closing ?> have a practical purpose.

  1. Avoid the use of var

var for declaring properties is a left over artifact from PHP 4's object model. PHP 5 and above support using public, private and protected for variables. I would suggest that your vars should be privates.

  1. Consider making vars consts or statics

You have several configuration member variables for your class. (comment_marker, bool_true_array, bool_false_array). The manner in which they are used suggest that they are constant properties of the class so it would provide clearer intent if they were changed to either constants or static variables.

If it is in fact a requirement that these variables can be changed per instance, consider passing their values into the object constructor and leaving them as private/protected member variables.

  1. Use a proper constructor

You use function config_parse() as a constructor, this is deprecated behaviour (I believe has been removed in PHP 7) change the method name to __construct and give it a visibility of public.

  1. Changes for file search

Consider moving the logic for file search into its own method this would allow child classes to provide their own implementation. You also need to be careful, your current implementation of parse file could lead to a security bug if you allow user input to be passed into the method. (consider /../../../etc/passwd being passed into that method)

  1. Else is rarely required

Consider process_config_line(), you have an if, elseif, else construct however each of the branches return. You could simply have two ifs and leave the last call unguarded. There are other places where this could be done as well (eg if you factor out the search method above)

  1. Consider re-factoring the search logic into a separate class

You could pull out the search logic into a separate class and then inject a finder class into the constructor. This improves separation of concerns and simplifies changing the finding method later.

  1. Have a think about what happens if you parse 2 files

Currently, you setup the config array in the constructor. If you parse two files, you will end up with the config from both in the return value for the second call. Consider if this is actually what you want.

Edit:

As an aside, your code style suggests you are using somewhat outdated reference materials for PHP, you might want to take a look at http://www.phptherightway.com/ to get up to speed with more modern practices.

\$\endgroup\$

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.