2
\$\begingroup\$

There is a tree of start folder, it's subfolders, their subfolders, etc. In each folder, subfolder, etc. there are the same structured XML files stored.

books.xml

<xml>
<book>
    <author>Isak Azimov</author>
    <name>End of spirit</name>
</book>
<book>
    <author>熱い</author>
    <name>Happiness of plants</name>
</book>
<book>
    <author>Isak Azimov</author>
    <name>Stars and rain</name>
</book>
<book>
    <author>Florea Paun</author>
    <name>Life journey</name>
</book>
</xml>
  • PHP script should read XML files information and add it to PostgreSQL two database tables: “authors” and “books” (use 1:many and unique author’s ID as link between the tables). XML files content should be displayed as a result.
  • If a record from specified file and subfolder already exists PHP script has to update the record and not to insert it as a new one.

Here is my try:

<?php

    $dsn = "pgsql:dbname=interpay host=localhost";
    $options = [
      PDO::ATTR_EMULATE_PREPARES   => false, 
      PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION, 
      PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, 
    ];
    try {
      $pdo = new PDO($dsn, "postgres", "4137", $options);
    } catch (Exception $e) {
      error_log($e->getMessage());
      exit('Fatal error!');
    }

    function tableExists($pdo, $table) {
        $table = preg_replace('/[^\da-z_]/i', '', $table);
        try {
            $result = $pdo->query("SELECT 1 FROM {$table} LIMIT 1");
        } catch (Exception $e) {
            return FALSE;
        }
        return $result !== FALSE;
    }

    $sql = <<<EOF
        CREATE TABLE authors
        (id SERIAL PRIMARY KEY,
        name TEXT NOT NULL);
    EOF;
    if (!tableExists($pdo, "authors"))
        $pdo->prepare($sql)->execute();

    $sql = <<<EOF
        CREATE TABLE books
        (id SERIAL PRIMARY KEY,
        author_id INT NOT NULL,
        name TEXT NOT NULL,
        CONSTRAINT fk_author FOREIGN KEY(author_id) REFERENCES authors(id));
    EOF;
    if (!tableExists($pdo, "books"))
        $pdo->prepare($sql)->execute();

    //XML file path
    $path = "books.xml";

    //Read entire file into string
    $xmlfile = file_get_contents($path);

    //Convert XML string into an object
    $new = simplexml_load_string($xmlfile);

    //Convert into json
    $con = json_encode($new);

    //Convert into associative array
    $booksArr = json_decode($con, true);

    for($i = 0; $i < count($booksArr['book']); $i++) {

        //Check if author already inserted into table
        $sql = <<<EOF
            SELECT * FROM authors WHERE name = ?;
        EOF;

        $stmt = $pdo->prepare($sql);
        $stmt->bindParam(1, $booksArr['book'][$i]['author'], PDO::PARAM_STR);
        $stmt->execute();
        $author_row = $stmt->fetch();

        if ($author_row)
            $last_author_id = $author_row['id'];
        else {
            $sql = <<<EOF
                INSERT INTO authors (name) VALUES (?);
            EOF;
            $stmt = $pdo->prepare($sql);
            $stmt->bindParam(1, $booksArr['book'][$i]['author'], PDO::PARAM_STR);
            $stmt->execute();
            echo "Author record created successfully!<br>";

            $sql = "SELECT * FROM authors ORDER BY id DESC LIMIT 1";
            $stmt = $pdo->prepare($sql);
            $stmt->execute();
            $author_row = $stmt->fetch();
            $last_author_id = $author_row['id'];
        }
        
        //Check if book inserted into table
        $sql = <<<EOF
            SELECT * FROM books WHERE name = ?;
        EOF;
        $stmt = $pdo->prepare($sql);
        $stmt->bindParam(1, $booksArr['book'][$i]['name'], PDO::PARAM_STR);
        $stmt->execute();
        $book_row = $stmt->fetch();

        //If book does not exist then insert into table
        if (!$book_row) {
            $sql = <<<EOF
                INSERT INTO books (author_id, name) VALUES (?, ?);
            EOF;
            $stmt = $pdo->prepare($sql);
            $stmt->bindParam(1, $last_author_id, PDO::PARAM_INT);
            $stmt->bindParam(2, $booksArr['book'][$i]['name'], PDO::PARAM_STR);
            $stmt->execute();
            echo "Book record created successfully!<br>";
        }
    }

    $pdo = null;
    $stmt = null;

?>
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

Code structure

That's a bit too much code for a single review. Given the task can be seamlessly separated into several modules, it will make it simpler to write, to read, to maintain and to review as well. For example, all the table creation code has no business with the main task of importing XML. The overall amount of code makes it harder to write a complete review, so I would suggest to post another question after taking into account all the suggestions given here. You see, a well-split code will produce a solid review and a solid code will produce a ragged review.

  • Usually, the table creation is supplied in the form of an SQL file or a migration but not in the form of PHP code.
  • the database connection code should be also moved into a separate file that just included in the main script
  • many SQL queries can be put into functions as not to pollute the main operation loop. Like, the code below //Check if author already inserted into table should be just one line, $author_row = getAuthor($pdo, $book['author']); All those functions again can be put into the included file and offered for the separate review. Or at least as a separate block of code in the same review.

Logical consistency.

  • It is possible that two authors can give their books the same name. It would make sense to check both the name and the author.
  • there is some folder structure mentioned but no code provided to handle it. A built-in recursive directory iterator would do though
  • you forgot the book update code. That's a direct result of your "wall of code" approach. With the main logic fits in one screen, it's easy to spot the mistake. While in such a wall of code it's easy to overlook one.

Formatting and readability

The overall formatting is good, your indentation is even and lines are reasonably long. Two suggestions:

  1. Do not create a default indent. The baseline of PHP statements should be a the very left position.
  2. Omit the closing PHP tag, it is forbidden by the standard
  3. Using bindParam is a bit verbose. You can make it into one line, $stmt->execute([$last_author_id,$book['name']]);
  4. The way you are using heredoc is way too verbose and doesn't actually use the benefits of heredoc. For my personal taste, heredoc is not needed here at all, and the entire assignment can be done in a single line using standard quotes: $sql = "SELECT * FROM authors WHERE name = ?"; - simple, compact and readable. But in case you prefer heredoc, at least move the closing tag to align with the opening tag, so there will be no extra spacing in the query

Error reporting

Using try catch around PDO connection is inconsistent and verbose. Given the error can occur on the every line, why only the PDO connection code gets such a special treatment? It just makes no sense. Besides, it's just convenient to see all errors on-screen dring development. You can (and should) configure the uniform error reporting for the entire PHP code. See the basic error reporting rules

Cache

You can avoid some roundtrips to database by storing known authors in the array like this $known_authors[$name] = $last_author_id; so next time you can try to get the id by simple $last_author_id = $known_authors[$name] ?? false; and only get to DB if it fails.

Other improvements.

  1. Why not simplexml_load_file()?

  2. The tableExists implementation is barbaric. There are more civilized ways to check the table existence than causing a deliberate error.

  3. Using SELECT query to get the autogenerated id is WRONG on so many levels. Not only it's too verbose as compared to the proper way which is just $last_author_id = $pdo->lastInsertId(); but using SELECT query is guaranteed to give wrong results.

  4. Come on, using for to iterate over arrays is prehistoric. I'd break my fingers trying to write

this

for($i = 0; $i < count($booksArr['book']); $i++) {
    echo $booksArr['book'][$i]['author'];
}

instead of just

foreach ($booksArr['book'] as $book) {
    echo $book['author'];
}
\$\endgroup\$
3
  • \$\begingroup\$ I'm not going to post a review. Perhaps you'd like to mention that /[^\da-z_]/i simplifies to /\W+/. Using a one-or-more quantifier makes potentially longer and fewer replacements. \$\endgroup\$ Oct 9, 2022 at 21:13
  • \$\begingroup\$ I would explain why "using for to iterate over arrays is prehistoric.". Just shaming others for using it isn't very polite. \$\endgroup\$
    – Dharman
    Oct 19, 2022 at 23:09
  • \$\begingroup\$ @Dharman I thought "breaking fingers" and the foreach example speak for themselves. And I didn't shame no one, but the method. \$\endgroup\$ Oct 20, 2022 at 5:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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