2
\$\begingroup\$

I need to make a simple CSV->MySQL script in OOP and I was wondering if there's anything I am doing wrong in a object-oriented sense.

I have a file name db.php where I connect to MySQL via MySQLi and then I have another class where I load a csv file into MySQL like this:

class csv extends Database {

        public $file = null;

   function __construct() {

            if(count($_SERVER["argv"]) > 1){
                $this->file = $_SERVER["argv"][1];
            }else{
                $this->file = "stock.csv";
            }

            $this->db = $db;

        }

        function saveCSVtoDB(){
            //opens the file
            $fp = fopen($this->file, "r");



            $db = Database::getInstance();
            $mysqli = $db->getConnection();

            $result = $mysqli->query("LOAD DATA LOCAL INFILE 'file.csv'
                                           IGNORE INTO TABLE products
                                               CHARACTER SET UTF8
                                        FIELDS TERMINATED BY ',' 
                                         LINES TERMINATED BY '\n'
                                              IGNORE 1 LINES
                                              (code, name, desc, stock, price, @avai)
                                                         SET available = IF(@avai LIKE '%yes%', 1, 0)
                                               ");

            if($result){
                echo "success";
            }else{
                die("error");
            }

        }
}
\$\endgroup\$
4
\$\begingroup\$

So I would recommend to not extend the CSV parser from the Database class. Instead The CSV parser should get the Database per dependency injection through the constructor. Also I would not recommend to access gobal vars like $_SERVER in a class. Here an example how you could do that:

class CSVParser
{
    protected $file;
    protected $database;

    public function __construct($file, Database $db)
    {
        $this->file = $file;
        $this->db = $db;
    }

    public function saveCSVtoDB()
    {
        // @TODO import CSV to DB
    }
}

And you could use the class with:

$file = isset($_SERVER["argv"][1]) ? $_SERVER["argv"][1] : "stock.csv";

$parser = new CSVParser($file, Database::getInstance());
$parser->saveCSVtoDB();
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Hard-coded Values

Your code contains a lot of hard coded values, from the file name to the columns you expect it to contain and even some custom logic if the form on the if statement inside the query.

Should you want to create a more generic, reusable solution, all these inputs should be customizable.

Importing CSVs like that has constraints

When you specify a filename to the relational database so that it imports data, the database server searches for that file relative to it's filesystem. It's quite common that the database server runs on a different machine than the one your web server runs under. This needs to be taken into consideration.

Given this information, you open the file in PHP for no apparent reason.

| improve this answer | |
\$\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.