HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PDO connection via INI parser classes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
parserviaclassesiniconnectionpdo

Problem

A few days ago I asked this question and I've got an answer to work with interfaces. I've tweaked around with this to get myself a INI parser class.

isValidated('file', $file)) {
            throw new Exception("File seems not to work..");
        }
        $credentials = parse_ini_file($file, true);
        foreach ($credentials as $key => $values) {
            if (!$this->isValidated('values', $credentials, $section)) {
                throw new Exception("Values or section doesn't exist");
            } elseif ($key !== $section) {
                continue;
            }
            $this->data = $values;
        }
    }

    /**
     * This function returns the value from a key inside a array.
     *
     * @param $key mixed Key of the array
     *
     * @throws Exception
     * @return mixed Value of the selected key by the parameter
     */
    public function get ($key) {
        if (!isset($this->data[$key])) {
            throw new Exception("Value doesn't exist in key: " . $key);
        }
        return $this->data[$key];
    }

    /**
     * @param $type
     * @param $data
     * @param string $section
     * @return bool
     *
     * note: when validating the values, were validating an array.
     */
    public function isValidated ($type, $data, $section = "") {
        switch ($type) {
            case "file":
                return file_exists($data) || is_file($data);
            case "values":
                //1. Check if $data is an array
                //2. Section is set
                return is_array($data) || isset($data[$section]);
        }
        return false;
    }
}


When calling the parse() method in this class you get a specific .ini file (first argument) and parses the values within a specific section.

When calling the get() method in this class, you get the value of the chosen key.

This are the required methods for it:

```
<?php

interface Parser
{
public function parse($file, $section);

public fun

Solution

Structure

Your IniParser should be the only part of your code that is aware of how the ini file looks internally. That's the whole point of the interface: With it, you can parse different kinds of files (or even retrieve the data from something else). If the file structure changes at some point, you should only ever have to change your IniParser class, nothing else.

But right now, that isn't the case. Your Connection also needs to know how your file looks. To fix this, I would add explicit getters instead of the generic getter you have right now. So you should add getEngine, getHost, etc to your class. This has the added benefit that you can validate them (see that they exist, etc) as soon as you parse the file, instead of when you are using the credentials.

Then of course your naming doesn't make that much sense anymore. So I would rename Parser to ICredentialsParser and IniParser to IniCredentialsParser.

You might also think about passing the names of keys to it as well instead of hardcoding them inside the parser.

If you do all this, your Connection class is decoupled from the ini file itself, and also, your IniParser gets more meaning (before, it was more or less only a wrapper for parse_ini_file).

parse and continue

If possible, I would avoid continue. In some cases it makes sense, but most of the time, it makes code harder to read. Your code could look like this:

if (!$this->isValidated('values', $credentials, $section)) {
            throw new Exception("Values or section doesn't exist");
        } elseif ($key == $section) {
            $this->data = $values;
        }


Misc

  • your isValidated('values', $credentials, $section) call is inside a loop, but doesn't use the loop variables, so you should move it out of the loop.



  • I would create separate isValidated functions, as they don't share any common code (the way you do it just complicates things). You can name them isValidFile and hasSection.



  • naming: most of the time, I see open and close for the operations you are naming initialize and terminate. Your names are not bad, just unconventional. I would go with the general convention.



  • naming: isValidated would sound a bit better as isValid.



  • isValidated should probably be private.



  • in PHPDocs, it's always good to write why an exception is thrown.

Code Snippets

if (!$this->isValidated('values', $credentials, $section)) {
            throw new Exception("Values or section doesn't exist");
        } elseif ($key == $section) {
            $this->data = $values;
        }

Context

StackExchange Code Review Q#64968, answer score: 5

Revisions (0)

No revisions yet.